RouteSecurityPolicyRegistry keeps two policy lists — AUTHORIZATION_POLICIES for
session/cookie access and API_TOKEN_POLICIES for Bearer tokens — with nothing
keeping them in step. Routes the authorization list opens but the token list
never registers fall through to the catch-all and answer
API token cannot access endpoint: <path>.
Register the routes reported in #713 (/api/v1/labels, the star and rating
writes) plus the same-class gaps for /api/v1/auth/methods and paths below
/api/v1/download, and add a guard test that walks the authorization list and
fails when a route is neither token-reachable nor declared session-only.
DELETE /api/v1/skills/{id}/star also matched the SUPER_ADMIN rule for
DELETE /api/v1/skills/*/*, so un-starring was refused for ordinary accounts on
the session path too. Star and rating writes now have their own authorization
entries ahead of that rule.
Closes#713
Signed-off-by: FenjuFu <fufenjupku@gmail.com>
* fix(security): publish scan task after transaction commit
SecurityScanService.triggerScan is @Transactional but published the Redis
Stream scan task inline, before the transaction committed. The stream
consumer could receive the task before the skill_version / security_audit
rows were visible, fail with "SkillVersion not found" / "SecurityAudit not
found", exhaust its immediate retries while the publishing transaction was
still open, and leave the committed version stuck in SCANNING.
Defer the publish to an afterCommit transaction synchronization so the
consumer only ever sees the task once the rows are committed and visible; on
rollback the task is never published. Falls back to an inline publish when
called outside a transaction.
Closes#612
Signed-off-by: FenjuFu <fufenjupku@gmail.com>
* test(security): cover scan task after-commit publishing
Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
* refactor(security): hide scan publish transaction callback
Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
---------
Signed-off-by: FenjuFu <fufenjupku@gmail.com>
Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
Co-authored-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
Downloading a skill whose slug is non-ASCII (e.g. a Chinese name) through
the ClawHub CLI compatibility route failed: the 302 Location header was
built by string-concatenating the raw slug, and Tomcat encodes header
values as ISO-8859-1, so a character outside 0-255 makes it drop the
Location header entirely and the download breaks. The skillhub CLI path
was unaffected because it doesn't go through this redirect.
Build the Location with UriComponentsBuilder.pathSegment(...).encode(), so
each segment is percent-encoded while the '/' separators stay literal.
"需求" becomes %E9%9C%80%E6%B1%82 and the header is ISO-8859-1-writable.
Fixes#658
Signed-off-by: FenjuFu <92919259+FenjuFu@users.noreply.github.com>
Remove terminal review tasks before deleting an allowed skill version. Lock all versions of the aggregate in stable order so concurrent deletes preserve the last-version invariant and return business errors instead of 500 responses.
Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
* fix(api): tell callers why a request was forbidden
The scope filter already computes an exact reason ("Missing API token
scope: skill:delete", "API token cannot access endpoint: /x") and the
access-denied handler discarded it, returning a bare "Forbidden" for
every case: missing scope, endpoint closed to API tokens, and paths
that simply don't exist. Clients cannot tell those apart, so they
guess — the published CLI reports every 403 as "token may lack
required scope", which sent us debugging token scopes for an hour when
the real causes were a revoked token and a mistyped namespace path.
The reason now rides in the response via a new error.forbidden.detail
message (en + zh), and is logged alongside the exception type.
Signed-off-by: Gal Eyal <gal.e@popai.health>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(api): safely expose API token denial reasons
Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
---------
Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
The shared RedisTemplate uses GenericJackson2JsonRedisSerializer with
the application ObjectMapper, which embeds no type information, so
stored DeviceCodeData deserializes as a LinkedHashMap. The typed casts
in pollToken and authorizeDeviceCode then throw ClassCastException on
every call, making the whole device authorization flow unusable
(every poll returns 500).
Convert the raw value with ObjectMapper.convertValue instead of
casting; this reads both the current untyped map format and any typed
format, so no stored-data migration is needed. Adds bean setters to
DeviceCodeData for map conversion and regression tests that feed the
service exactly what Redis returns in production (untyped maps).
Fixes#604
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Gal Eyal <gal.e@popai.health>
* fix(publish): delete review tasks of any status when replacing a version
Re-uploading a rejected version under the same version number returned
HTTP 500. deleteReplaceableVersionArtifacts only removed a PENDING review
task, but a rejected version owns a REJECTED one; that row kept a foreign
key on the skill_version, so the subsequent delete hit a constraint
violation that surfaced as a 500.
Delete every review task attached to the version instead.
Signed-off-by: FenjuFu <92919259+FenjuFu@users.noreply.github.com>
* test(publish): drop the spring-test dependency from the new test
skillhub-domain has no spring-test on its test classpath, so
ReflectionTestUtils does not resolve there. Use plain JDK reflection for
setting the generated id and invoking the private method.
Signed-off-by: FenjuFu <92919259+FenjuFu@users.noreply.github.com>
* fix(publish): constrain rejected version replacement
Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
* test(publish): verify replaced review is deleted
Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
* test(e2e): use generated API response types
Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
---------
Signed-off-by: FenjuFu <92919259+FenjuFu@users.noreply.github.com>
Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
Co-authored-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
The @Async rebuildSkill fix relied on a fresh thread giving a clean
transaction boundary. But skillhubEventExecutor uses CallerRunsPolicy:
under saturation the rejected task runs on the caller (request) thread,
back inside the afterCommit synchronization phase — the original failure
context where the @Transactional index write is silently dropped.
Mark SearchIndexService.index as REQUIRES_NEW so it always suspends any
lingering post-commit synchronization and commits in its own transaction,
independent of whether the async dispatch actually happened.
Add regression tests: detach removes the label keyword, and a synchronous
rebuild inside the afterCommit phase still persists the document (fails
without REQUIRES_NEW).
Signed-off-by: shychee <shychee96@gmail.com>
Attaching or detaching a skill label triggers a search index rebuild via
an afterCommit callback. Because LabelSearchSyncService.rebuildSkill ran
synchronously on the request thread, the @Transactional index write
executed inside the already-committed transaction-synchronization phase
and was silently dropped -- the search document was never written, so
label keywords never became searchable.
Move rebuildSkill onto the skillhubEventExecutor with @Async (matching the
existing rebuildSkills batch path) so the rebuild runs on a fresh thread
and transaction. Add an integration test that fails on the old synchronous
path and passes with the async fix.
Signed-off-by: shychee <shychee96@gmail.com>
