Skip to content

WW-5688 Resolve id-bearing REST URIs into the root namespace when declared (6.x backport) - #1863

Open
lukaszlenart wants to merge 1 commit into
support/struts-6-x-xfrom
WW-5688-rest-root-namespace-6x
Open

WW-5688 Resolve id-bearing REST URIs into the root namespace when declared (6.x backport)#1863
lukaszlenart wants to merge 1 commit into
support/struts-6-x-xfrom
WW-5688-rest-root-namespace-6x

Conversation

@lukaszlenart

Copy link
Copy Markdown
Member

Backport of PR #1862 to support/struts-6-x-x, targeting 6.12.0.

Fixes WW-5688, split out of the 2008 umbrella WW-2820 (originally reported by Alvin Singh).

The bug

RestActionMapper mapped a URI carrying an id into the default namespace, while mapping the same action without an id into /. Because getActionConfig() only fails over from / to "" and never the other way round, an action declared in a package with namespace="/" resolved for index but 404'd for show, update and destroy.

The fix

DefaultActionMapper has handled this since WW-2461 (June 2008) via a rootAvailable check — three months before WW-2820 reported the REST symptom. It was never ported to the copy of parseNameAndNamespace() the REST plugin forked earlier. This ports it, keeping the ordering that computes the action name while the namespace is still empty, since the name is a substring relative to it.

The production change is byte-identical to the one on mainparseNameAndNamespace() had not diverged between the two branches. The only backport-specific edit is the new test's import (com.opensymphony.xwork2.XWorkTestCase instead of the org.apache.struts2 one).

Verified against this branch, not assumed from main

Each claim was re-checked here rather than carried over:

  • RED reproduced on 6.x independently. With the source change reverted on this branch, show/update/destroy fail while index and id-extraction pass — the same 3 failures as on main. Restoring the fix turns them green.
  • DefaultConfiguration.shouldFallbackToEmptyNamespace here has the same one-way /"" behaviour (core/.../DefaultConfiguration.java:605), which is what makes the wrong namespace a dead end rather than a soft miss.
  • The Convention plugin on this branch likewise derives "" or /sub and never /, so it cannot trigger the promotion on its own, and apps/rest-showcase declares no namespace on its package — unaffected.

Blast radius

The promotion fires only when a package explicitly declares namespace="/" and nothing more specific matched. Because a / lookup already falls back to "", the set of resolvable actions is a strict superset of the previous one — nothing that resolved before stops resolving. No change to core, no new configuration surface.

Tests

Full rest plugin suite on this branch: 80 tests, 0 failures.

Not a security fix

This makes actions less reachable rather than more, and aligning /dog/1 with what /dog already resolves to exposes no surface the /dog path does not already expose.

🤖 Generated with Claude Code

…en declared

RestActionMapper mapped a URI carrying an id into the default namespace while
mapping the same action without an id into "/". Since the configuration only
fails over from "/" to "" and never the other way round, an action declared in
a package with namespace="/" resolved for index but 404'd for show, update and
destroy.

DefaultActionMapper already handles this: WW-2461 added a rootAvailable check
in June 2008, three months before WW-2820 reported the REST symptom, but the
fix was never ported to the mapper the REST plugin had forked earlier. Port it,
keeping the ordering that computes the action name while the namespace is still
empty, since the name is relative to it.

The promotion only fires when a package explicitly declares namespace="/" and
nothing more specific matched. Convention derives "" or "/sub" and never "/",
so applications that do not opt in are unaffected, and because a "/" lookup
already falls back to "", the set of resolvable actions is a superset of the
previous one.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant