fix(api): stop the skill-pack routes discarding what a caller sent - #941
Open
brahyam wants to merge 1 commit into
Open
fix(api): stop the skill-pack routes discarding what a caller sent#941brahyam wants to merge 1 commit into
brahyam wants to merge 1 commit into
Conversation
Four silent failures across the two routes, all the same shape: a 200 that
throws away or destroys the field it was given.
PATCH ignored authCredentialSlug, which POST accepts. So a pack registered
against a private repository without a credential, or with the wrong one,
could never be given the right one — the request answered 200 with the pack
unchanged and the only way forward was to delete the pack and register it
again, which archives its imported skills. It now sets the slug, clears it on
null or a blank string, and 400s on anything else.
Neither route validated the slug against isValidCredentialSlug, already
enforced where a credential is created and in the git HTTP broker.
`Repo-Token` stored fine and could never match anything, and resolvePackAuth
answers undefined for a slug it cannot resolve rather than throwing, so the
next sync fetched the private repository unauthenticated. registerPack also
stored the slug untrimmed while patchPack trimmed it. Both routes now share
one reader.
config was destroyed by a malformed value on PATCH and dropped by one on
POST: asConfig returned a fresh object keeping only well-typed fields, and
DurableMap.merge deletes a key whose patch value is undefined, so
PATCH {"config":"oops"} answered 200 and silently wiped the pack's exclude
globs — after which the next sync imported the skills the operator had
excluded.
Guarding the outer shape is not enough, and that is worth recording because
it was the first attempt: {"exclude": "trusted/*"}, a string where an array
belongs and the natural single-glob mistake, is a well-formed object, so it
passed and asConfig still returned {}. asConfig is now a parse-or-error
reader like the slug's, so a wrong type INSIDE the config is a 400 naming the
field, and the shape predicate beside it is deleted rather than extended.
The audit event for a credential change carries the slug in detail, on both
routes. Which credential a pack may use is a security-relevant binding;
without it a re-point was indistinguishable from a syncMode flip and a pack
registered with a credential recorded nothing at all.
A slug is deliberately NOT checked for existence: registering a pack before
creating its credential is a legitimate order, so a 400 on an unknown slug
would refuse valid work. The failure that would catch belongs where the
credential is resolved.
Both new tests register against the file's existing local fixture repository
rather than a remote URL, so neither reaches DNS.
This is the API only. The admin offers the field on the register form alone
and does not display which credential a pack uses.
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
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.
What
Four silent failures across the two skill-pack routes, all the same shape: a 200 that throws away or destroys the field it was given.
PATCHignoredauthCredentialSlug, whichPOSTaccepts. A pack registered against a private repository without a credential, or with the wrong one, could never be given the right one — the request answered 200 with the pack unchanged, and the only way forward was to delete the pack and register it again, which archives its imported skills. It now sets the slug, clears it onnullor a blank string, and 400s on anything else.Neither route validated the slug against
isValidCredentialSlug, which is already enforced where a credential is created and in the git HTTP broker.Repo-Tokenstored fine and could never match anything, andresolvePackAuthanswersundefinedfor a slug it cannot resolve rather than throwing — so the next sync fetched the private repository unauthenticated.registerPackalso stored the slug untrimmed whilepatchPacktrimmed it. Both routes now share one reader.configwas destroyed by a malformed value onPATCHand dropped by one onPOST.asConfigreturned a fresh object keeping only well-typed fields, andDurableMap.mergedeletes a key whose patch value isundefined— soPATCH {"config":"oops"}answered 200 and silently wiped the pack'sexcludeglobs, after which the next sync imported the skills the operator had excluded.Checking only the outer shape is not enough, and that is worth stating because it was the first attempt at this fix:
{"exclude": "trusted/*"}— a string where an array belongs, which is the natural single-glob mistake — is a well-formed object, so it passed, andasConfigstill returned{}.asConfigis now a parse-or-error reader like the slug's, so a wrong type inside the config is a 400 naming the field, and the shape predicate beside it is gone rather than extended.The audit event for a credential change carries the slug in
detail, on both routes. Which credential a pack may use is a security-relevant binding; without it a re-point was indistinguishable from asyncModeflip, and a pack registered with a credential recorded nothing at all.Deliberately not included
A slug is not checked for existence. Registering a pack before creating its credential is a legitimate order, so a 400 on an unknown slug would refuse a valid workflow. The failure it would catch — a typo'd slug quietly fetching a private repository unauthenticated — is better addressed where the credential is resolved, and is what the companion change to the fetcher's failure message reports: it names the slug and says the credential is missing, disabled, or env-delivered.
The admin UI is unchanged. It offers the field on the register form only and does not display which credential a pack uses, so this correction is reachable by API and not yet from the page. That is a front-end change and wants a demo with it.
Verified
test/skill-packs-routes.test.tscovers setting a slug, replacing one, clearing withnulland"", a patch that does not mention the field, a non-string, three unstorable shapes with the stored value unchanged after each, the same rule on the register route including a padded slug, a malformedconfigon both routes at the outer and inner level, and that theexcludeglobs survive every refused patch.Both new tests register against the file's existing local fixture repository rather than a remote URL, so neither reaches DNS.
npm run typecheck, lint,prettier --check,lint:knipand the full suite pass.Related: the companion change to the fetcher's failure message referred to above is #939.