refactor(artifact): parse token fields without regex split - #545
refactor(artifact): parse token fields without regex split#545seonghobae wants to merge 10 commits into
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| final String payload = token.substring(0, lastDot); | ||
| final String expectedSignature = hmac(payload); | ||
| final String signature = token.substring(lastDot + 1); |
There was a problem hiding this comment.
🔍 Malformed-token fast path regresses
parseAndVerify computes HMAC before validating field count. Dotted malformed tokens now incur cryptographic work that the previous parser skipped.
Was this helpful? React with 👍 or 👎 to provide feedback.
![cwl-noema-review[bot]](https://avatars.githubusercontent.com/u/295022177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Noema LLM review
The token parsing optimization replaces String.split with indexOf/substring, but it computes HMAC over the payload before validating the token's field count. Malformed dotted tokens therefore incur cryptographic work that the previous parser skipped, introducing a denial-of-service regression on malformed token paths.
Reviewed changed lines
src/main/java/com/clearfolio/viewer/artifact/ArtifactLinkService.java:342 (RIGHT): HMAC is computed from token.substring(0, lastDot) before validating that the payload contains exactly TOKEN_FIELD_COUNT - 1 dots. Malformed tokens with fewer fields still trigger cryptographic work.src/main/java/com/clearfolio/viewer/artifact/ArtifactLinkService.java:343 (RIGHT): The HMAC comparison occurs before field-count validation, so a token like 'a.b.c' reaches the hmac() call instead of failing fast as the previous split-based parser did.src/main/java/com/clearfolio/viewer/artifact/ArtifactLinkService.java:344 (RIGHT): Signature extraction happens before validating the payload field structure, reinforcing the malformed-token fast path regression described in the prior review thread.
Adversarial validation
src/main/java/com/clearfolio/viewer/artifact/ArtifactLinkService.java:342 (RIGHT)confirmed: The optimized parser computes HMAC before validating the field count. — Lines 342-344 compute hmac(payload) and extract the signature before the for loop validates that exactly TOKEN_FIELD_COUNT - 1 dots exist.src/main/java/com/clearfolio/viewer/artifact/ArtifactLinkService.java:353 (RIGHT)falsified: Field-count validation still rejects malformed tokens before any additional processing. — The later loop does reject missing dots, but only after HMAC and signature comparison have already executed.- Residual risk: Malformed short tokens now invoke HMAC-SHA-256 computation before field-count validation, enabling cheap malformed inputs to consume cryptographic CPU and worsening rejection latency.
Findings
- [high] src/main/java/com/clearfolio/viewer/artifact/ArtifactLinkService.java:342 (RIGHT): HMAC is computed over the payload before validating the token field count. Malformed dotted tokens now incur cryptographic work that the previous split-based parser skipped, enabling a cheap-input CPU consumption regression. Validate that the payload contains exactly TOKEN_FIELD_COUNT - 1 dots before calling hmac(payload).
- Result: REQUEST_CHANGES
- Head SHA:
c908c26b192ecd64ec201b4f2eaf58c5e5feb3ca - Reviewer credential:
noema-review-github-app-refresh - Actor:
cwl-noema-review[bot]
seonghobae
changed the title
1 participant
Current exact authority
main@06633a25109c62e24a7015ae04fb9f6e0a246f7e36ad9b64af0b1013b147fd2aea236cb0639add60ahead_by=5,behind_by=0)src/main/java/com/clearfolio/viewer/artifact/ArtifactLinkService.javaVerified semantic delta
ArtifactLinkService.parseAndVerify()replacesString.split("\\.", -1)plusArrays.copyOf/String.joinwith indexed delimiter discovery and explicit field extraction. The token retains the same signed payload/signature boundary, exact field-count requirement, HMAC verification, Base64URL/UUID/epoch decoding, and authorization error mapping.This is a plausible allocation-oriented refactor. It is not, by itself, evidence of lower GC pressure, lower allocation, higher throughput, or buyer-visible latency improvement. Existing
ArtifactLinkServiceTestcoverage establishes behavioral compatibility for valid and malformed token paths; it is not a performance benchmark.Review → repair
The generated lane mixed the source refactor with two claims that were stronger than the available evidence:
.jules/bolt.mdpromoted this local implementation choice into repository-wide performance doctrine;CHANGELOG.mddescribed the parser as a performance optimization that reduces GC burden without JMH/JFR/allocation or representative latency evidence.Normal descendants repaired both without rewriting history:
f2b4f7b768f133acb5be0880dbfb66cecb2234e2restores.jules/bolt.mdbyte-for-byte to protectedmain;36ad9b64af0b1013b147fd2aea236cb0639add60restoresCHANGELOG.mdto protectedmain, removing the unmeasured buyer-facing performance claim.The branch ref advanced with
force=false. Fresh protected-base compare now contains only the production parser refactor.Next causal acceptance
If this is to be promoted as a performance change rather than a neutral refactor, add reproducible evidence on representative valid and malformed tokens under the same JVM/JIT/GC conditions, including allocation bytes/op (or equivalent JFR/JMH evidence) and latency/throughput. Preserve exact semantics for too few/many separators, empty fields, empty signature, malformed Base64URL, wrong signature, and admitted request-size bounds.
If measured benefit is negligible, review the indexed parser only on semantic/maintainability grounds; do not manufacture a performance claim from test coverage.
Exact-head gate state
Fresh workflows on
36ad9b64af0b1013b147fd2aea236cb0639add60are non-terminal: CI33966421119, fuzz33966421078, Security Scan33966421173, SAST Semgrep33966421081, CodeQL PR33966421136are queued. Predecessor checks/reviews do not transfer.Keep Draft until one unchanged exact head has terminal applicable CI/security/SAST/fuzz/review evidence and ordinary protected-branch eligibility. No force-push, no-op retrigger, self-approval, gate weakening, or unmeasured performance claim.