fix(cdk): normalize after cursor case + colon-truncation in approval-scope guard#360
Open
AshrafBen10 wants to merge 6 commits into
Open
fix(cdk): normalize after cursor case + colon-truncation in approval-scope guard#360AshrafBen10 wants to merge 6 commits into
AshrafBen10 wants to merge 6 commits into
Conversation
…roval-scope guard
Two correctness bugs in the task API handlers:
1. get-task-events: a lower-case `after` ULID cursor passed validation
(isValidUlid uppercases before matching) but was used verbatim in the
DynamoDB key condition `event_id > :after`. Stored event_ids are
upper-case Crockford Base32 and DDB compares raw bytes, so a lower-case
cursor sorts after every stored id and the query returns zero events —
a consumer using a contract-valid lower-case cursor silently skips the
rest of the event stream. Fix: upper-case the cursor before the query.
2. create-task-core: the degenerate-pattern guard for bash_pattern:/
write_path: scopes used scope.split(':', 2)[1], which truncates the
value at the next colon. A value whose leading colon-segment is short
or wildcard-heavy (e.g. "ab:cdefgh") was truncated to a degenerate
fragment and spuriously rejected with a 400. Fix: take everything after
the first colon via slice(indexOf(':') + 1).
Adds regression tests for both paths.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #360 +/- ##
=======================================
Coverage ? 88.50%
=======================================
Files ? 219
Lines ? 51897
Branches ? 4861
=======================================
Hits ? 45933
Misses ? 5964
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
|
@AshrafBen10 thanks ! Almost ready, please run a full build locally and push the changes not committed (see failing build step on mutation), we should be good to go after that |
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
4 participants
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
Fixes two correctness bugs in the task API handlers, each with a regression test.
Bug 1 — lower-case
afterULID cursor silently returns zero eventscdk/src/handlers/get-task-events.tsisValidUlidaccepts lower-case callers (it uppercases before matching), and the API contract documents case-insensitive cursors. But the handler fed the raw cursor into the DynamoDB key conditionevent_id > :after. Storedevent_ids are upper-case Crockford Base32; DynamoDB compares raw bytes, and lower-case ASCII sorts after upper-case. So a lower-case cursor is greater than every stored id → the query returns zero events, and a consumer silently skips the rest of the event stream (data loss in replay).Fix: upper-case the cursor before the query.
Bug 2 — colon in a glob value breaks the degenerate-pattern guard
cdk/src/handlers/shared/create-task-core.tsThe guard for
bash_pattern:/write_path:scopes usedscope.split(':', 2)[1], which truncates the value at the next colon. A value whose leading colon-segment is short or wildcard-heavy (e.g.ab:cdefgh) was truncated to a degenerate fragment (ab) and spuriously rejected with a 400.Fix: take everything after the first colon via
slice(indexOf(':') + 1).Testing
test/handlers/get-task-events.test.ts— added lower-case cursor normalization testtest/handlers/shared/create-task-core.test.ts— added colon-value accept + genuine-degenerate reject tests