feat: validate resourceDependencies at write time, gated by allowUserResourceDependencies - #1941
Open
serguei-gorokhov wants to merge 3 commits into
Open
serguei-gorokhov wants to merge 3 commits into
serguei-gorokhov wants to merge 3 commits into
Conversation
…ResourceDependencies A new ResourceDependencyValidator enforces the two-form target language (concrete global-view path, or current-user placeholder rooted), the allowed-targets rule, the ~100-entry cap, and record shape — a pointer rule only, no permission on the target is checked or required. It runs on both application writer surfaces: ResourceController (user/public bucket PUTs) and ConfigResourceController (platform bucket), which previously bypassed application validation entirely. The governance ceiling: user-authored apps may not declare dependencies while the new settings flag allowUserResourceDependencies is off (the default); with it on, personal targets must be typed — a root-level current-user/ declaration is not declarable. Static settings are not hot-reloaded. BlobEntityValidator gains the read-side warning form of the same rules, reusing one implementation. Note: CustomApplicationApiTest.testApplicationListing fails on Windows on pristine development alike (a pre-existing gpt-backslash folder issue, green on CI's Linux runners) — unrelated to this change. Spec: documentation repo, offline-access-delegation/implementation-specs/pr2-write-time-validation.md Co-Authored-By: Claude Code <noreply@anthropic.com>
Security-review follow-up to the write-time validation commit: the token bans (.., *, current-user off the root) ran on raw string segments, while the platform canonicalizes declared paths through ResourceDescriptorFactory's single tryDecodePath pass when building descriptors. Percent-encoded smuggles (%2e%2e, %2a, %63urrent-user, %75sers) therefore passed shape validation and would decode to exactly the banned tokens at resolution time — a validate-then-normalize mismatch inside the control this series introduces. The validator now decodes each segment the same way the factory does (one tryDecodePath pass, mirroring fromEntityPath) before applying the token and root-form rules; legitimately encoded segments such as my%20folder remain declarable. Co-Authored-By: Claude Code <noreply@anthropic.com>
Review-driven fixes to the validation commit:
- Bounded work: once a section exceeds the 100-entry cap only the first
100 entries are inspected, so a huge body cannot turn validation into
unbounded allocation and a giant 400 message.
- Slash-only target paths (zero segments after split) are a 400 shape
error instead of an ArrayIndexOutOfBounds 500.
- A bare type root ("files", "public/") is rejected — it addresses a
whole global view, as over-broad as the personal root the ceiling bans.
- The governance ceiling is keyed on the author
(accessService.hasAdminAccess) rather than the destination bucket:
an admin prototyping in their own bucket authors an admin app.
- The bulk admin-apply surface (ConfigApplyService.applyApplication)
now applies the same shape rules, returning a FAILED entity result —
validate identically on every write path.
- Platform-bucket PUT: shape validation runs before the underBucketLocks
critical section — pure CPU work does not belong inside a cluster-wide lock.
- Removed the BlobEntityValidator warning backstop added in the previous
commit: BlobEntityValidator.validate has zero main-code callers
(pre-existing dead code), so the claimed read-side mitigation was
unreachable. The residual surface is config-file apps only —
admin-authored by definition, and treated as unresolvable at resolution.
- README settings table documents allowUserResourceDependencies.
Co-Authored-By: Claude Code <noreply@anthropic.com>
serguei-gorokhov
requested review from
Oleksii-Klimov and
astsiapanay
as code owners
September 9, 2026 18:30
This was referenced Sep 9, 2026
astsiapanay
requested changes
Sep 11, 2026
| } | ||
| // Once over the cap the section is rejected anyway — inspect only the first MAX entries so a | ||
| // huge body cannot turn validation itself into unbounded allocation. | ||
| int inspected = Math.min(section.size(), MAX_DECLARED_DEPENDENCIES); |
Collaborator
There was a problem hiding this comment.
Why do we need to call the min function?
We already know the size is less or equal to MAX cap
| return issues; | ||
| } | ||
|
|
||
| private static List<String> pathIssues(String at, ResourceDependency dependency) { |
Collaborator
There was a problem hiding this comment.
the logic should rely on ResourceDescriptorFactory
| "files": [], | ||
| "reload": 60000, | ||
| "onInvalidEntity": "abort", | ||
| "allowUserResourceDependencies": false, |
Collaborator
There was a problem hiding this comment.
the setting's name says it could be applied to any resource in fact applications have it.
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
2 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.
Applicable issues
Description of changes
A new
ResourceDependencyValidatorenforces the two-form target language (concrete global-view path, or current-user placeholder rooted), the allowed-targets rule, the ~100-entry cap, and record shape — a pointer rule only, no permission on the target is checked or required. It runs on both application writer surfaces:ResourceController(user/public bucket PUTs) andConfigResourceController(platform bucket), which previously bypassed application validation entirely.The governance ceiling: user-authored apps may not declare dependencies while the new settings flag
allowUserResourceDependenciesis off (the default); with it on, the personal root is not declarable. The ceiling is keyed on the author (accessService.hasAdminAccess), not the destination bucket — an admin prototyping in their own bucket authors an admin app. Static settings are not hot-reloaded. README settings table documents the flag.The story ships as three commits; the two review-hardening commits are part of this PR's intended diff:
d59c25b5— decoded-segment bans: the token bans (..,*, placeholder off the root) run on decoded segments, mirroring the descriptor factory's singletryDecodePathpass, so percent-encoded smuggles (%2e%2e,%2a,%63urrent-user,%75sers) are caught at validation instead of decoding into banned tokens at resolution time; legitimately encoded segments (my%20folder) remain declarable.2feee1d0— bounded work and coverage: over-cap sections inspect only the first 100 entries (no unbounded allocation, no giant 400 message); slash-only target paths are a 400 shape error instead of anArrayIndexOutOfBounds500; a bare type root (files,public/) is rejected as over-broad; the bulk admin-apply surface (ConfigApplyService.applyApplication) applies the same shape rules (FAILED entity result); platform-bucket shape validation runs before theunderBucketLockscritical section.Note:
CustomApplicationApiTest.testApplicationListingfails on Windows on pristinedevelopmentalike (a pre-existing gpt-backslash folder issue, green on CI's Linux runners) — unrelated to this change.Behavior changes flagged for review
allowUserResourceDependencies, default off — user-authored declarations are impossible until an operator turns it on; static settings, restart to change.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
🤖 Generated with Claude Code