app: Fix ARN encoding in web launcher URL#64827
Conversation
juliaogris
added
no-changelog
juliaogris
force-pushed
the
julia/app/aws-rolepicker-fix
branch
2 times, most recently
from
7dbf35b to
2ac8725
Compare
juliaogris
force-pushed
the
julia/app/aws-rolepicker-fix
branch
from
9f568d2 to
f6d0658
Compare
|
@codex - could I have a review please? |
|
Codex Review: Didn't find any major issues. 👍 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
juliaogris
force-pushed
the
julia/app/aws-rolepicker-fix
branch
from
cc7813d to
2f8e005
Compare
This comment was marked as outdated.
This comment was marked as outdated.
When an AWS console app role ARN contains a `/` in the path component (e.g. `arn:aws:iam::123:role/my-role`), the web launcher redirect truncates the ARN at the slash. This is a regression from the react-router v7 upgrade (PR #63496). Both v5 and v7 encode `/` as `%2F` in the URL path via `generatePath`. The difference is in `useParams()`: v5 returned the raw percent-encoded value, v7 auto-decodes it. In v5, `useParams().arn` was still percent-encoded (e.g. `role%2Fmy-role`). `getNewAuthExchangeUrl` put that encoded value into the query string, so Go's `q.Get("arn")` decoded one layer and got `role%2Fmy-role` (literal `%2F`, not a slash). `path.Join` treated `%2F` as ordinary characters and the full ARN survived. In v7, `useParams().arn` is auto-decoded to `role/my-role` (literal `/`). `getNewAuthExchangeUrl` puts that decoded value into the query string, Go's `q.Get("arn")` gets a literal `/`, and `path.Join` in `makeAppRedirectURL` splits the ARN at that slash. The redirect URL carries a truncated ARN, the session cert is issued with the truncated value, and the access check rejects the mismatch. Percent-encode the ARN with `url.PathEscape` before appending it to the URL path, and replace `path.Join` with `strings.Join` plus explicit `RawPath`/`Path` setting so that `%2F` survives URL serialization.
juliaogris
force-pushed
the
julia/app/aws-rolepicker-fix
branch
from
2f8e005 to
f79b396
Compare
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
@claude review please |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
My previous concern about the PathUnescape fallback has been addressed — all segments are now url.PathEscaped before building RawPath, making the error path unreachable. LGTM.
Extended reasoning...
Overview
This PR fixes a regression introduced by the react-router v7 upgrade: useParams() in v7 auto-decodes percent-encoded path segments, causing ARNs with / to be split at the slash during the redirect round-trip. The fix applies url.PathEscape to all URL path segments on the Go side and sets u.RawPath explicitly so that %2F survives serialization.
Security risks
The changed code handles user-controlled input (ARN, cluster name, public address) in URL construction. The url.PathEscape applied to all segments prevents injection of bare slashes or invalid percent sequences into the redirect URL path. No auth bypass or privilege escalation vectors are introduced.
Level of scrutiny
This is a focused regression fix in the app launcher URL construction path. The logic is well-documented with a detailed step-by-step breakdown in the PR description, and the fix is confirmed by bisection. The change is confined to makeAppRedirectURL in handler.go and corresponding JS/test updates.
Prior review resolution
My previous inline comment flagged that an intermediate version had a u.Path = u.RawPath fallback on PathUnescape error which would corrupt the ARN encoding. The author addressed this by applying url.PathEscape to all path segments (not just the ARN) before building RawPath. Since url.PathEscape guarantees valid percent-encoding, url.PathUnescape now always succeeds and the discarded error is genuinely unreachable. The Codex P2 comment about unescaped non-ARN segments was resolved by the same change.
Escape every segment in the redirect URL path, not just the ARN. This prevents any unexpected percent sequences in cluster names or public addresses from being emitted raw in the Location header.
juliaogris
force-pushed
the
julia/app/aws-rolepicker-fix
branch
from
3e196d4 to
12cbf72
Compare
There was a problem hiding this comment.
LGTM — the url.PathEscape-all-segments approach I recommended has been correctly implemented and the broken fallback removed.
Extended reasoning...
Overview
This PR fixes a regression in the AWS console app launcher introduced by the react-router v5→v7 upgrade. The core issue: react-router v7's useParams() auto-decodes percent-encoded path segments (e.g. %2F→/), so ARNs with slashes were being truncated at the slash on the second round-trip. The fix is applied in Go (makeAppRedirectURL) by re-escaping the decoded ARN with url.PathEscape before embedding it in RawPath, and by switching from path.Join (which treats / structurally) to strings.Join (which preserves literal %2F). The JS side already has the right behavior since useParams() decodes the segment for createAppSession.
Security risks
No security risks. The changed code builds a redirect URL for the app launcher flow. URL encoding is tightened (all segments now go through url.PathEscape), which if anything reduces exposure to path-traversal via crafted cluster or publicAddr values. No authentication logic, crypto, or authorization checks are modified.
Level of scrutiny
Medium. The change touches URL construction in the app auth redirect flow, which is a user-visible path but not a security boundary itself. The PR description includes a detailed step-by-step trace of the encoding flow across all three scenarios (v5, v7-broken, v7-fixed), which is unusually thorough and makes the correctness easy to verify.
Other factors
My previous review identified that the intermediate version (with u.Path = u.RawPath fallback) was actively wrong — it would double-encode %2F to %252F when triggered. The author resolved this exactly as I suggested: apply url.PathEscape to all non-ARN segments so the error path is never reachable and the fallback was removed. The inline comment was resolved and Codex also gave a clean pass on the updated revision. New tests cover multi-level ARN paths, special characters, and a round-trip test through React Router routing. No bugs found by the bug hunting system.
public-teleport-github-review-bot
Bot
removed request for
alexhemard,
gabrielcorado and
zmb3
* app: Fix ARN encoding in web launcher redirect URL When an AWS console app role ARN contains a `/` in the path component (e.g. `arn:aws:iam::123:role/my-role`), the web launcher redirect truncates the ARN at the slash. This is a regression from the react-router v7 upgrade (PR #63496). Both v5 and v7 encode `/` as `%2F` in the URL path via `generatePath`. The difference is in `useParams()`: v5 returned the raw percent-encoded value, v7 auto-decodes it. In v5, `useParams().arn` was still percent-encoded (e.g. `role%2Fmy-role`). `getNewAuthExchangeUrl` put that encoded value into the query string, so Go's `q.Get("arn")` decoded one layer and got `role%2Fmy-role` (literal `%2F`, not a slash). `path.Join` treated `%2F` as ordinary characters and the full ARN survived. In v7, `useParams().arn` is auto-decoded to `role/my-role` (literal `/`). `getNewAuthExchangeUrl` puts that decoded value into the query string, Go's `q.Get("arn")` gets a literal `/`, and `path.Join` in `makeAppRedirectURL` splits the ARN at that slash. The redirect URL carries a truncated ARN, the session cert is issued with the truncated value, and the access check rejects the mismatch. Percent-encode the ARN with `url.PathEscape` before appending it to the URL path, and replace `path.Join` with `strings.Join` plus explicit `RawPath`/`Path` setting so that `%2F` survives URL serialization. * app: Percent-encode all path segments in launcher redirect Escape every segment in the redirect URL path, not just the ARN. This prevents any unexpected percent sequences in cluster names or public addresses from being emitted raw in the Location header.
3 participants
When an AWS console app role ARN contains a
/in the path component (e.g.arn:aws:iam::123:role/my-role), the web launcher redirect truncates the ARNat the slash.
The configured IAM role ARN must appear exactly in the session cert for the
access check to pass. When the cert carries a truncated value
(
arn:aws:iam::123:roleinstead ofarn:aws:iam::123:role/my-role), the appservice rejects the mismatch and the user gets "Not Found".
This is a regression from the react-router v7 upgrade
(PR gravitational/teleport#63496).
The app launcher route is
/web/launch/:fqdn/:clusterId?/:publicAddr?/:arn?.Both v5 and v7 encode
/as%2FingeneratePath. The difference isuseParams(): v5 (via path-to-regexp 1.9.0) returned the raw percent-encodedvalue, while v7 (which dropped path-to-regexp) auto-decodes it. This changes
how the ARN travels through the auth exchange.
Confirmed by bisecting between the commit before the v7 upgrade (c75ea42)
and the upgrade commit itself (6298fe2). The
cert.createaudit event showsthe difference:
Percent-encode the ARN with
url.PathEscapebefore appending it to the URLpath, and replace
path.Joinwithstrings.Joinplus explicitRawPath/Pathsetting so that
%2Fsurvives URL serialization.react-router v5 vs v7 vs v7 with this fix
v5 (react-router 5.3.4, path-to-regexp 1.9.0):
generatePathencodes/→%2F. URL:.../arn%3Aaws%3Aiam%3A%3A123%3Arole%2Fmy-roleuseParams()does NOT decode →params.arn=arn%3Aaws%3Aiam%3A%3A123%3Arole%2Fmy-role🔑searchParams.set('arn', params.arn)double-encodes →arn=arn%253A...%252Fmy-roleq.Get("arn")decodes one layer →arn%3Aaws%3Aiam%3A%3A123%3Arole%2Fmy-rolepath.Joinsees%2Fas literal characters, not a slash → ARN stays as one segment ✅%2Fintact:arn?matches the whole thing because%2Fis not a/✅decodeURIComponent(params.arn)→arn:aws:iam::123:role/my-rolecreateAppSessionPOSTsarn: "arn:aws:iam::123:role/my-role"aws_role_arn = arn:aws:iam::123:role/my-rolev7 broken (react-router 7.13.0, no path-to-regexp):
generatePathencodes/→%2F. URL: same as v5useParams()auto-decodes%2F→params.arn=arn:aws:iam::123:role/my-role(literal/) 💥searchParams.set('arn', params.arn)single-encodes →arn=arn%3A...role%2Fmy-roleq.Get("arn")decodes →arn:aws:iam::123:role/my-role(literal/)path.Joinoutputs path with literal/inside the ARN, indistinguishable from structural slashes.../arn:aws:iam::123:role/my-role?state=TOKEN:arn?matches[^/]+, captures onlyarn:aws:iam::123:role.my-rolebecomes a stray segment, discarded 💥decodeURIComponentis a no-op on the already-truncated valuecreateAppSessionPOSTsarn: "arn:aws:iam::123:role"aws_role_arn = arn:aws:iam::123:rolev7 with this fix:
generatePathencodes/→%2F. URL: same as aboveuseParams()auto-decodes%2F→params.arn=arn:aws:iam::123:role/my-role(literal/)searchParams.set('arn', params.arn)single-encodes →arn=arn%3A...role%2Fmy-roleq.Get("arn")decodes →arn:aws:iam::123:role/my-role(literal/)url.PathEscape(req.arn)re-encodes/→%2F🔧strings.Joinpreserves%2Fas literal characters (replacespath.Join) 🔧u.RawPathset with encoded value.url.URL.String()usesRawPath→%2Fintact 🔧%2Fin path.:arn?matches the whole encoded segment.useParams()auto-decodes → full ARN ✅decodeURIComponent(params.arn)is a no-op (already decoded)createAppSessionPOSTsarn: "arn:aws:iam::123:role/my-role"aws_role_arn = arn:aws:iam::123:role/my-roleIssue: #64823
Manual Test Plan
Test Environment
Local Teleport instance with an AWS console app configured with an IAM
role whose ARN contains a path component
(
arn:aws:iam::<ACCOUNT>:role/<ROLE_NAME>).Test Cases
awsconsoleapp, select the role fromthe picker. The AWS console opens instead of "Not Found".
round-trip through
makeAppRedirectURL).tsh apps login awsconsole --aws-role <ARN>followedby
tsh aws sts get-caller-identityreturns the assumed role.