Add internal paginated location catalog for Sheets and extension - #190
Add internal paginated location catalog for Sheets and extension#190doobneek wants to merge 8 commits into
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR introduces a new authenticated internal catalog endpoint, but the current authz model appears too permissive and test coverage does not validate the risky production auth path.
Blocking issues
- Potential broken access control in
src/services/internal-location-catalog-auth.js: any valid Cognito token from the configured user pool is accepted (token_usein {id, access}) without audience/client-id/group/scope checks. If the pool contains non-internal users, they can access/locations/catalog. - Host allowlist enforcement is bypassable as a security boundary in
assertAllowedRequestHost(src/services/internal-location-catalog-auth.js): whenOrigin/Refererare absent, requests are allowed, so non-browser clients with a valid token bypass host restrictions entirely. - Missing high-risk auth tests:
test/integration/find-locations.test.jsonly exercisesNODE_ENV=testbypass and does not test real JWT verification/claim enforcement (issuer, signature failure, token type, and authorization constraints). This leaves production auth behavior unclear.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d6b9357af
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| app.use(morgan('dev')); | ||
|
|
||
| app.use(cors()); | ||
| app.use(cors({ exposedHeaders })); |
There was a problem hiding this comment.
/locations/catalog is meant for browser clients (sheets.doobneek.org and the extension), but this change only updates Express CORS. I checked simple-proxy-api.yaml, and both mock OPTIONS handlers still return Access-Control-Allow-Origin: 'https://example.com' (lines 43-50 and 86-93). Because the new route requires an Authorization header, browsers will preflight it and reject requests from the intended origins before they ever reach Express, so the feature is unusable outside server-side callers.
Useful? React with 👍 / 👎.
|
Addressed the review feedback in aaab98d.
I also scanned the current branch diff plus the PR body/comments for token-like secrets and did not find any exposed bearer/API tokens, so no history rewrite was needed. |
There was a problem hiding this comment.
Not safe to approve: there are security control gaps in the new internal catalog auth path and at least one likely validation regression that is untested.
Blocking issues
assertAllowedRequestHostonly enforces the host allowlist forhttp/httpsorigins and also skips checks whenOrigin/Refererare absent. This creates an origin-policy bypass for non-http origins (for examplenull/extension/file contexts) and weakens the intended browser restriction on a sensitive endpoint.src/config.jsintroduces hardcoded default Cognito pool/client authorization values (DEFAULT_COGNITO_USER_POOL_ID,DEFAULT_INTERNAL_CATALOG_ALLOWED_CLIENT_IDS). This is fail-open auth configuration: if env config is missing/mis-set, the endpoint still authorizes tokens against baked-in identities.sortByvalidation insrc/controllers/validation/locations.jsusesJoi.string().valid([ ... ]), which likely validates against a single array literal instead of allowed string values. This is a likely API regression and there is no integration test coveringsortBybehavior.
|
Addressed the latest review feedback in 13e0e5e.
Validation run on this commit:
|
There was a problem hiding this comment.
Not safe to approve: the PR introduces an authentication bypass path in production code and a broad CORS policy relaxation with security/regression risk.
Blocking issues
- High severity auth bypass:
src/services/internal-location-catalog-auth.jsacceptsBearer test-internal-tokenwheneverNODE_ENV==='test'. Keeping this shortcut in application auth logic creates a hardcoded backdoor if environment config is ever wrong (or reused outside test harness). This should be removed from runtime code and handled via test-only mocking/fixtures. - Security regression risk:
simple-proxy-api.yamlchangesAccess-Control-Allow-Originfrom a fixed origin to'*'on API Gateway mock OPTIONS responses. This broadens cross-origin access for the whole API surface and is not covered by targeted security tests/policy justification in this PR.
There was a problem hiding this comment.
I do not consider this safe to approve yet due to authorization and regression risk concerns.
Blocking issues
src/services/internal-location-catalog-auth.js: Origin enforcement is treated as an auth gate, butOrigin/Refererare client-controlled headers and can be spoofed outside browsers; this creates unclear and potentially over-trusting access behavior for an internal endpoint.src/services/internal-location-catalog-auth.js: Access can be granted based on allowed client ID alone (group check optional), which may over-authorize if that client is broadly used; for an internal catalog endpoint this is a risky default and needs stricter documented constraints.simple-proxy-api.yaml: CORSAccess-Control-Allow-Originwas changed globally to a single hardcoded origin (https://sheets.doobneek.org) for API Gateway mock OPTIONS responses, which is a potential breaking regression for other browser consumers and is not covered by regression tests.
There was a problem hiding this comment.
The PR adds a sensitive internal endpoint with custom auth, but the auth cache implementation introduces a denial-of-service risk that is not safe to approve as-is.
Blocking issues
src/services/internal-location-catalog-auth.jsuses an unbounded in-memorytokenCachekeyed by raw bearer token, and entries are never actively evicted (expired entries remain unless the same token is seen again). Because caching happens before client/group authorization, any valid Cognito token from the issuer can be used to grow process memory indefinitely. This is a high-severity availability risk (memory exhaustion/DoS) for a public-facing endpoint.
There was a problem hiding this comment.
Not safe to approve: there is a CORS-policy bypass on the new internal catalog endpoint due to path matching logic, and risky path-variant behavior is not covered by tests.
Blocking issues
- Security/CORS bypass: in
src/app.js, CORS middleware selection usesif (req.path === '/locations/catalog'). Express will still route/locations/catalog/(and potentially other equivalent path variants) to the same handler, but those requests will fall back topublicApiCors(allow-all) instead ofinternalLocationCatalogCors, defeating the origin allowlist for browser requests. - Missing regression test for path-variant CORS behavior: tests cover allowed/disallowed origins for
/locations/catalogbut do not verify that/locations/catalog/(or equivalent variants) enforces the same restricted CORS policy. Given this is security-sensitive routing logic, this gap is blocking.
There was a problem hiding this comment.
Not safe to approve yet. The new catalog query has a pagination correctness risk that can cause skipped/duplicated locations across pages, and tests do not cover this high-risk case.
Blocking issues
src/models/location.js(Location.findCatalog): applyinglimit/offsettogether with ahasManyinclude (PhysicalAddress) can paginate SQL join rows instead of distinct locations. In Sequelize this often causes unstable paging (duplicate parent rows on one page, missing parents on later pages). This is a behavioral regression risk for/locations/catalogpagination.test/integration/find-locations.test.js: new catalog tests do not cover locations with multiplePhysicalAddresses, so the pagination/join risk above is untested. Per conservative review rules, risky pagination logic without regression tests is blocking.
1 participant
Summary
Validation
Notes