Skip to content

fix(security): use constant-time byte comparison for process token revocation (#1039) - #1085

Closed
KhushiTrivediii wants to merge 10 commits into
risa-labs-inc:mainfrom
KhushiTrivediii:fix/process-token-registry-constant-time-compare
Closed

KhushiTrivediii wants to merge 10 commits into
risa-labs-inc:mainfrom
KhushiTrivediii:fix/process-token-registry-constant-time-compare

Conversation

@KhushiTrivediii

Copy link
Copy Markdown
Contributor

Fixes #1039.

Replaces short-circuiting String == comparison with constant-time byte comparison (MessageDigest.isEqual) in ProcessTokenRegistry.revokeIfCurrent.

@choksi2212

Copy link
Copy Markdown
Contributor

Cross-referencing for the maintainer's triage — the file list here is 25 files, but the title describes a one-line change (MessageDigest.isEqual in ProcessTokenRegistry). The gap between title and diff matters for review, so mapping what the other 24 files actually are:

The diff re-implements fixes that are already open PRs by other contributors. Spot-verified against the diffs:

Two of this author's other open PRs carry the same 25-file list (#1102 loopback binding shows the identical file set under a different one-line title), which suggests both were branched from the same working tree and the titles describe only a fraction of what each diff changes.

For the merge decision: the PR's nominal subject (constant-time token compare) is sound and covered better by #1040 (which cites its own issue, #1039's predecessor, and carries the token-regression tests). Everything else in this diff duplicates open PRs whose authors filed the underlying issues, wrote the first fixes, and have test suites in review. A maintainer merging this as-titled would unknowingly land twenty-four files of parallel implementations that then conflict with every one of those PRs at rebase.

Suggested action: either rebase this branch down to its stated subject only (ProcessTokenRegistry.kt + its test), or close in favor of #1040 — the hackathon's judging criteria name "duplicated submissions" as explicitly not counted, and this diff is the definition of one.

@fluck-boss

Copy link
Copy Markdown
Collaborator

Thanks so much for this contribution, @KhushiTrivediii - the work here is genuinely appreciated, and it's clear real care went into it.

This overlaps with #1040 ("fix(ipc): compare the revoke-if-current token in constant time (#1039)"), which is further along in review and will carry this change forward. To keep review attention focused, I'm closing this one in favor of #1040 - please do review and follow that PR, and you're very welcome to pick up any part of it that still needs help or take on another open issue.

Your contribution credit is noted here, and anything unique in this branch that #1040 misses is still welcome as a follow-up. Thanks again for contributing to BossConsole!

@fluck-boss fluck-boss closed this Sep 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ProcessTokenRegistry.revokeIfCurrent compares a credential token with == (use constant-time compare)

3 participants