Skip to content

Fix allowedAction comparison and root enforcement - #90

Merged
dlongley merged 6 commits into
mainfrom
fix-allowed-action-comparison
Sep 17, 2026
Merged

dlongley merged 6 commits into
mainfrom
fix-allowed-action-comparison

Conversation

@jameseaster

@jameseaster jameseaster commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Compares allowedAction sets rather than JSON types so a delegation that narrows nothing succeeds whether each side spells its actions as a string or an array, and seeds parentAllowedAction from the root zcap so a root's allowedAction is enforced by the verifier and not only when a delegation proof is generated.

Fixes #89

@jameseaster jameseaster changed the title Fix allowed action comparison Fix allowedAction comparison and root enforcement Sep 15, 2026
@codecov-commenter

codecov-commenter commented Sep 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.10%. Comparing base (80cbe01) to head (105adc4).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #90      +/-   ##
==========================================
+ Coverage   90.66%   91.10%   +0.44%     
==========================================
  Files           6        6              
  Lines        1897     1901       +4     
==========================================
+ Hits         1720     1732      +12     
+ Misses        177      169       -8     
Files with missing lines Coverage Δ
lib/CapabilityInvocation.js 85.67% <100.00%> (ø)
lib/CapabilityProofPurpose.js 98.49% <100.00%> (ø)
lib/utils.js 87.11% <100.00%> (+1.26%) ⬆️

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80cbe01...105adc4. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread CHANGELOG.md Outdated
## 10.0.0 - 2026-09-dd

### Changed
- **BREAKING**: Enforce a root capability's `allowedAction` when verifying a

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be a patch. A delegated zcap that was invalid that this library didn't catch (even if properly signed by the delegator) is still invalid and this patch will now properly reject it. Those zcaps shouldn't have been accepted to begin with, so this is a fix.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated changelog here: 98fdac6

@jameseaster
jameseaster force-pushed the fix-allowed-action-comparison branch from 27f9f38 to 9941f20 Compare September 16, 2026 18:29
Comment thread lib/utils.js
const actions = getAllowedActions({capability: {allowedAction}});

// parent's `allowedAction` must include every one from child's
return actions.every(a => parentActions.includes(a));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test that covers an explicitly provided, but empty array for allowedAction? Added tests here should at least include negative tests where the child uses an empty array but the parent used a string or a non-empty array. It seems like an empty array in the child could pass here when it shouldn't.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, that the helper itself would return true there, but an empty array can't reach it: checkCapability rejects any allowedAction that isn't a string or non-empty array (utils.js#L578) and runs first on both paths - before hasValidAllowedAction in CapabilityDelegation.update (L145 vs L160), and on every zcap in the dereferenced chain (utils.js#L339, #L412) before _verifyCapabilityChain ever compares actions - which the three tests in 105adc4 now pin down at both layers.

@dlongley
dlongley merged commit cc2f631 into main Sep 17, 2026
5 checks passed
@dlongley
dlongley deleted the fix-allowed-action-comparison branch September 17, 2026 19:26
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.

hasValidAllowedAction rejects an array child against a string parent, even for an identical action set

3 participants