fix: surface awsUiAuth in /config so frontend Cognito login flow triggers#4610
Open
leonarduk wants to merge 9 commits into
Open
fix: surface awsUiAuth in /config so frontend Cognito login flow triggers#4610leonarduk wants to merge 9 commits into
leonarduk wants to merge 9 commits into
Conversation
#276 All three paths in `_resolve_ticker_exchange` now validate `sym` and `ex` against `_TICKER_SEGMENT_RE` before returning, breaking the taint flow that CodeQL's py/reflective-xss (CWE-079) analysis tracked from the `ticker` query parameter to the response. Removes the now-redundant `html.escape` from the JSON response path (HTML-encoding inside JSON is incorrect for non-HTML consumers and was only there as a CodeQL workaround). Closes #4561 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…th dots PR 4569's approach of applying _TICKER_SEGMENT_RE to resolver-returned values breaks valid portfolio entries with dotted exchange codes (e.g. XLON.G) and conflicts with three existing tests that document the intentional asymmetry: - test_resolve_inferred_path_trusts_internal_data: asserts "XLON.G" passes - test_timeseries_meta_html_xss_ticker_from_resolver_is_escaped: asserts resolver-returned XSS is HTML-escaped (not rejected with 400) - test_timeseries_meta_html_xss_exchange_from_resolver_is_escaped: same XSS defence for the resolver path is already provided by: - render_timeseries_html: html.escape(title) for the page heading - render_timeseries_html: df.to_html(escape=True) for table cells Restore html.escape() in the JSON response path (removed by PR 4569) to satisfy CodeQL's reflective-XSS check on this HTMLResponse-typed route. User-supplied ticker/exchange (cases 1 and 2) retain strict _TICKER_SEGMENT_RE validation; the new XSS-rejection tests in test_timeseries_meta_route.py all pass because they test user-supplied inputs only. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… check The three parametrised cases in test_timeseries_meta_xss_payload_rejected all hit _TICKER_SEGMENT_RE validation before any data loading, so they can only return 400, not 404. Accepting 404 would let a response that echoes the unescaped payload in an error body pass the test. Add the content-type check to confirm the rejection is JSON (not HTML), so the reflected input cannot be interpreted as markup by a browser. Addresses DeepSeek review feedback (test assertion too permissive). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…lective-xss #276 Add _RESOLVER_OUTPUT_RE (^[A-Z0-9._\-]{1,100}$) applied to sym/ex returned by _resolve_full_ticker. This breaks the taint flow from the user-supplied ticker query parameter to the HTML response that CodeQL tracked as alert #276. The wider pattern permits dots so valid dotted exchange codes (e.g. XLON.G) are accepted, while HTML-unsafe characters (<, >, &, ", ') are still rejected, closing the XSS vector even if the resolver returns unexpected data. Update test_timeseries_meta_html_xss_*_from_resolver_is_escaped → _is_rejected: the defence is now Layer 1 (validation → 400) rather than Layer 2 (HTML escaping → 200 with entities). test_resolve_inferred_path_trusts_internal_data docstring updated to reflect _RESOLVER_OUTPUT_RE semantics. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…gers The backend /config response was missing the awsUiAuth block, so the frontend's ensureAwsUiAuth() call did nothing and every protected API call went out without an Authorization header, causing 401s on /owners and /groups. Changes: - backend/config.py: add AwsUiAuthConfig dataclass; load UI_AUTH_DOMAIN and UI_AUTH_CLIENT_ID env vars; include in Config.aws_ui_auth - backend/contracts_spa.py: add AwsUiAuthContract; add optional awsUiAuth field (alias) to ConfigContract - backend/routes/config.py: serialise_config transforms aws_ui_auth to camelCase awsUiAuth block; only emits it when enabled=True - cdk/stacks/backend_lambda_stack.py: add UiAuthDomain CfnParameter; pass UI_AUTH_DOMAIN and UI_AUTH_CLIENT_ID to backend Lambda via add_environment() after params are defined - .github/workflows/deploy-lambda.yml: read UiAuthDomain from StaticSiteStack outputs; pass as --parameters to cdk deploy Closes #4577 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds ChangesAWS UI Auth config propagation
Timeseries meta endpoint XSS fix
Sequence DiagramsequenceDiagram
participant Client as Frontend Browser
participant ConfigEndpoint as Backend /config
participant Lambda as Backend Lambda
participant EnvVars as Lambda Environment
Client->>ConfigEndpoint: GET /config
ConfigEndpoint->>Lambda: load_config()
Lambda->>EnvVars: read UI_AUTH_DOMAIN<br/>read UI_AUTH_CLIENT_ID
EnvVars-->>Lambda: domain, client_id values
Lambda->>Lambda: construct AwsUiAuthConfig<br/>(enabled if both present)
Lambda-->>ConfigEndpoint: Config with aws_ui_auth
ConfigEndpoint->>ConfigEndpoint: serialise_config()<br/>convert aws_ui_auth to awsUiAuth
ConfigEndpoint-->>Client: JSON with awsUiAuth block
Client->>Client: ensureAwsUiAuth()<br/>redirect to Cognito if enabled
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
DeepSeek AI Code Review1. Acceptance criteriaThe diff satisfies all acceptance criteria from the linked issue:
The remaining AC items (redirect to Cognito, token storage, 200 responses) are frontend-side behaviours that depend on this backend fix — they're correctly scoped as downstream effects. 2. Bugs and logic errorsNo blocking bugs found. One minor observation: in 3. API, data, and workflow safetyNo blocking concerns.
4. Test coverageAdequate for the acceptance criteria.
Missing but non-blocking: No test verifies that the 5. Suggested follow-up issues (non-blocking)
APPROVE Reviewed by DeepSeek via deepseek-pr-review.yml. Advisory only. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cdk/stacks/backend_lambda_stack.py (1)
348-368: 🧹 Nitpick | 🔵 Trivial | 🏗️ Heavy liftSplit
BackendLambdaStack.__init__before extending parameter wiringThe constructor is already significantly oversized; adding more deployment/config branches here makes stack behaviour harder to reason about and test. Please extract auth-parameter/env wiring into a dedicated helper.
As per coding guidelines, “Function length must be ~60 lines maximum; refactor before adding more to an oversized function.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cdk/stacks/backend_lambda_stack.py` around lines 348 - 368, Extract the auth-parameter and environment variable wiring logic from the BackendLambdaStack.__init__ method into a dedicated helper method. Create a new private method (e.g., _configure_auth_parameters or _setup_ui_auth) that encapsulates the CfnParameter creation for ui_auth_domain_param and ui_auth_client_id_param, along with the corresponding backend_fn.add_environment calls for UI_AUTH_DOMAIN and UI_AUTH_CLIENT_ID. Call this new helper method from __init__ to keep the constructor focused and maintainable.Source: Coding guidelines
backend/config.py (1)
397-404: 🧹 Nitpick | 🔵 Trivial | 🏗️ Heavy liftRefactor
load_configbefore adding more env-mapping logicThis function is already far beyond the repository’s size limit; adding more config branches here increases maintenance risk. Please extract the UI auth env parsing into a helper and keep
load_configas an orchestrator.As per coding guidelines, “Function length must be ~60 lines maximum; refactor before adding more to an oversized function.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/config.py` around lines 397 - 404, The load_config function exceeds the recommended maximum function length and adding more configuration branches increases maintenance risk. Extract the UI auth environment variable parsing logic (the code that reads UI_AUTH_DOMAIN and UI_AUTH_CLIENT_ID and creates the AwsUiAuthConfig object) into a separate helper function. Create a new helper function that encapsulates this UI auth configuration parsing and returns the AwsUiAuthConfig, then replace the inline logic in load_config with a call to this helper function to keep load_config focused as an orchestrator.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@backend/config.py`:
- Around line 397-404: The load_config function exceeds the recommended maximum
function length and adding more configuration branches increases maintenance
risk. Extract the UI auth environment variable parsing logic (the code that
reads UI_AUTH_DOMAIN and UI_AUTH_CLIENT_ID and creates the AwsUiAuthConfig
object) into a separate helper function. Create a new helper function that
encapsulates this UI auth configuration parsing and returns the AwsUiAuthConfig,
then replace the inline logic in load_config with a call to this helper function
to keep load_config focused as an orchestrator.
In `@cdk/stacks/backend_lambda_stack.py`:
- Around line 348-368: Extract the auth-parameter and environment variable
wiring logic from the BackendLambdaStack.__init__ method into a dedicated helper
method. Create a new private method (e.g., _configure_auth_parameters or
_setup_ui_auth) that encapsulates the CfnParameter creation for
ui_auth_domain_param and ui_auth_client_id_param, along with the corresponding
backend_fn.add_environment calls for UI_AUTH_DOMAIN and UI_AUTH_CLIENT_ID. Call
this new helper method from __init__ to keep the constructor focused and
maintainable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cc78dbc0-f5df-4dbb-997c-18cd6984896f
📒 Files selected for processing (11)
.github/workflows/deploy-lambda.ymlbackend/config.pybackend/contracts_spa.pybackend/routes/config.pybackend/routes/timeseries_meta.pycdk/stacks/backend_lambda_stack.pycdk/tests/test_backend_lambda_stack.pytests/backend/routes/test_config.pytests/routes/test_timeseries_meta.pytests/test_config.pytests/test_timeseries_meta_route.py
DeepSeek AI Code Review1. Acceptance criteriaThe diff satisfies all acceptance criteria:
2. Bugs and logic errorsNo blocking bugs found. The logic is sound:
3. API, data, and workflow safetyNo blocking concerns. Key safety points:
4. Test coverageAdequate coverage for the acceptance criteria. The three new tests cover:
One minor gap: there's no integration test verifying that the CDK parameter wiring actually produces the correct env vars in the Lambda function. However, this is acceptable given the CDK test validates the template structure and the backend tests validate the runtime behavior. 5. Suggested follow-up issues (non-blocking)
APPROVE Reviewed by DeepSeek via deepseek-pr-review.yml. Advisory only. |
…dRouter Starlette 1.x (lambda-compat job, backend/requirements.txt) returns _IncludedRouter objects in app.routes alongside Route objects; these lack a .path attribute and caused AttributeError on the set comprehension. Filter with isinstance(route, APIRoute) — the same pattern used in tests/backend/test_smoke_endpoint_list.py — so the test is robust across both 0.49.x and 1.x starlette versions. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
DeepSeek AI Code Review1. Acceptance criteriaThe diff satisfies all acceptance criteria:
The remaining AC items (redirect behavior, token storage, 200 responses) are frontend-side behaviors that depend on this backend fix — they cannot be verified from this diff alone but are correctly enabled by it. 2. Bugs and logic errorsNo blocking issues found. The logic is sound:
3. API, data, and workflow safetyNo blocking issues found. Key safety considerations are handled:
4. Test coverageAdequate coverage for the acceptance criteria:
Missing but non-blocking: No integration test verifies the full end-to-end flow (CDK → Lambda env → 5. Suggested follow-up issues (optional)
Reviewed by DeepSeek via deepseek-pr-review.yml. Advisory only. |
In Starlette 1.x (backend/requirements.txt) include_router wraps routes in _IncludedRouter objects; filtering with isinstance(route, APIRoute) silently excludes them and causes the rebalance route to be missed. Switch to posting to /rebalance via TestClient and asserting the response is not 404 — this is robust across all Starlette versions. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
AwsUiAuthConfigdataclass tobackend/config.py, populated from newUI_AUTH_DOMAINandUI_AUTH_CLIENT_IDenv varsserialise_config()inbackend/routes/config.pynow emits a camelCaseawsUiAuthblock when both env vars are set, giving the frontend theenabled,domain, andclientIdfields it needs to start the Cognito PKCE login flowbackend/contracts_spa.pygetsAwsUiAuthContract+ an optionalawsUiAuthfield onConfigContractUiAuthDomainCfnParameter toBackendLambdaStack; passesUI_AUTH_DOMAINandUI_AUTH_CLIENT_IDto the Lambda viaadd_environment()after the parameters are definedUiAuthDomainfromStaticSiteStackoutputs and passes it as a--parametersargument tocdk deployTest plan
tests/test_config.py::test_aws_ui_auth_loads_from_env—AwsUiAuthConfigpopulated from env varstests/test_config.py::test_aws_ui_auth_disabled_when_env_absent— defaults toenabled=Falsewhen env absenttests/backend/routes/test_config.py::test_read_config_includes_aws_ui_auth_when_env_set—awsUiAuthblock present with correct camelCase keystests/backend/routes/test_config.py::test_read_config_omits_aws_ui_auth_when_disabled—awsUiAuthabsent when not configuredtests/backend/test_spa_response_contracts.py—ConfigContract.model_validatestill passes with optionalawsUiAuthCloses #4577
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
/configendpoint now exposes AWS UI authentication asawsUiAuth(includingdomainandclientId) when enabled.Security
/timeseries/metaticker/exchange validation to block XSS-style payloads, returning HTTP 400 for invalid formats.Tests
/timeseries/metainputs.awsUiAuthis omitted when not configured.