Feat/paperless attachement links#1492
Open
szaiser wants to merge 32 commits into
Open
Conversation
Contributor
Summary by CodeRabbit
WalkthroughThis PR adds Paperless-backed attachment integration endpoints and client UI, shared outbound HTTP validation, default entity-type bootstrapping with retries, attachment MIME/tracing updates, preference sync extraction, and multiple frontend data-shape changes for parent-based location handling and template fields. ChangesBackend integration proxy and validation
Estimated code review effort: 4 (Complex) | ~60 minutes Entity-type default bootstrapping
Estimated code review effort: 3 (Moderate) | ~25 minutes Attachment MIME registry and trace redaction
Estimated code review effort: 3 (Moderate) | ~20 minutes Frontend integration UI
Estimated code review effort: 3 (Moderate) | ~25 minutes Frontend entity/template data-shape updates
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant V1Controller
participant OutboundTransport
participant PaperlessServer
Client->>V1Controller: GET /entities/{id}/attachments/integration-cards
V1Controller->>V1Controller: load entity, user settings, filter attachments
V1Controller->>OutboundTransport: build request with validated URL
OutboundTransport->>PaperlessServer: fetch document metadata (Token auth)
PaperlessServer-->>OutboundTransport: JSON metadata / error
OutboundTransport-->>V1Controller: response
V1Controller-->>Client: integration cards (ok/error state)
Client->>V1Controller: GET .../integration-thumbnail
V1Controller->>V1Controller: validate attachment link + scope
V1Controller->>OutboundTransport: fetch thumbnail
OutboundTransport->>PaperlessServer: GET thumbnail
PaperlessServer-->>OutboundTransport: image bytes
V1Controller-->>Client: streamed image with safe Content-Type
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
⬆️ enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (6)
frontend/composables/use-preferences.test.ts (1)
38-43: 💤 Low valueConsider using valid theme values in test data.
Line 39 uses
theme: "dark" as never, which suggests "dark" may not be a validDaisyThemevalue. Usingas neverbypasses type checking and reduces test quality.Recommend either:
- Use a valid theme value from the
DaisyThemetype (e.g., "homebox")- If "dark" is actually valid, remove the
as neverassertionNote: Line 67 in a later test uses
theme: "dark"withoutas never, which is inconsistent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/composables/use-preferences.test.ts` around lines 38 - 43, The test uses an invalid cast "theme: \"dark\" as never" which hides type errors; update the test in use-preferences.test.ts to supply a valid DaisyTheme or remove the cast: change the prefs object (currently spread from DEFAULT_PREFERENCES) passed to buildSyncedSettings(SYNC_ALL) so theme is either a real DaisyTheme value (e.g., "homebox" or whatever is defined in the DaisyTheme union) or, if "dark" is valid, drop the `as never` and use `theme: "dark"`. Ensure consistency with the later test that already uses `theme: "dark"` without a cast and keep references to DEFAULT_PREFERENCES, buildSyncedSettings, and SYNC_ALL unchanged.frontend/composables/preferences-utils.ts (1)
89-101: 💤 Low valueConsider improving type safety instead of using
as never.The
as neverassertion on line 97 bypasses TypeScript's type checking entirely, which could hide runtime type errors if the server returns unexpected value types for preference keys.Consider one of these approaches:
- Add runtime type validation before assignment
- Define a more specific type for the
settingsparameter that constrains it to valid preference value types- At minimum, add a comment explaining why this assertion is safe
The current implementation works but sacrifices type safety for convenience.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/composables/preferences-utils.ts` around lines 89 - 101, mergeSyncedSettings currently silences TypeScript by casting settings[key] to never before assigning to nextPreferences; instead make the assignment type-safe by either (A) narrowing the settings parameter to a keyed type that maps known preference keys to their allowed value types (e.g., Record<keyof LocationViewPreferences, ...>) so you can assign without assertions, or (B) perform a runtime type-check per key inside mergeSyncedSettings using a small type-check map/type guard for each preference key (validate settings[key] matches the expected type) and only then assign to nextPreferences; remove the `as never` cast and replace it with the validated value (or tighten the function signature) and keep forEachSyncedPreference usage the same.frontend/pages/profile.vue (1)
515-520: ⚡ Quick winConsider adding a password visibility toggle for the API token field.
Users often need to verify they've pasted their token correctly. The token is stored in settings (not as sensitive as a password that's never stored), so adding a show/hide toggle would improve usability without significantly increasing security risk.
This is already implemented for password fields elsewhere in the app (see
FormPasswordcomponent with toggle). You could either:
- Use
FormPasswordcomponent which has built-in toggle- Add a similar toggle to the token field
Example using
FormPassword:<FormPassword v-model="integrationSettings.paperlessToken" :label="$t('profile.paperless_token')" :placeholder="$t('profile.paperless_token_placeholder')" />🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/pages/profile.vue` around lines 515 - 520, Replace the current FormTextField usage for the API token with a field that supports visibility toggling (either swap to the existing FormPassword component or add the same toggle behavior) so users can show/hide integrationSettings.paperlessToken when verifying input; update the component at the FormTextField instance in profile.vue (the block rendering integrationSettings.paperlessToken with label profile.paperless_token and placeholder profile.paperless_token_placeholder) to use FormPassword or mirror its show/hide logic and props.backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go (1)
41-128: ⚖️ Poor tradeoffConsider: Rate limiting per user for proxy endpoint.
Since users control the upstream URL, they could abuse this endpoint to:
- Hammer a third-party service with requests
- Use the proxy as a generic HTTP client
- Cause excessive outbound bandwidth usage
While the existing auth middleware provides per-user isolation, consider adding per-user rate limiting for this endpoint specifically.
Example approach:
// In routes.go, wrap the handler with a rate limiter r.Get("/integrations/{name}/proxy", chain.ToHandlerFunc( v1Ctrl.HandleIntegrationProxy(), append(userMW, a.integrationProxyLimiter.middleware)..., ), )This is lower priority than the timeout/size fixes but worth considering for production hardening.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go` around lines 41 - 128, Add per-user rate limiting to the integration proxy by wrapping the V1Controller.HandleIntegrationProxy handler with a user-scoped limiter middleware: implement or reuse an integrationProxyLimiter that keys limits by the authenticated user ID (services.UseUserCtx(ctx.Context).ID) and attach its middleware when registering the route (e.g., in routes.go wrap the GET "/integrations/{name}/proxy" with chain.ToHandlerFunc(v1Ctrl.HandleIntegrationProxy(), append(userMW, a.integrationProxyLimiter.middleware)...)); ensure the limiter runs before the handler and returns a proper 429/Retry-After response when the per-user quota is exceeded.frontend/components/Item/AttachmentsList.vue (2)
105-107: ⚡ Quick winHardcoded service name in i18n string.
The service name
"Paperless"is hardcoded in the i18n interpolation. If additional services are added in the future, this will need to be updated manually.Consider deriving the service name from the attachment's MIME type via the adapter registry:
♻️ Suggested refactor for dynamic service names
Add a helper to get the display name:
+function getServiceDisplayName(attachment: ItemAttachment): string { + const adapter = SERVICE_ADAPTERS.find(a => a.mimeType === attachment.mimeType); + return adapter ? adapter.name.charAt(0).toUpperCase() + adapter.name.slice(1) : "Service"; +}Then use it in the template:
<TooltipContent> - {{ $t("components.item.attachments_list.open_in_service", { service: "Paperless" }) }} + {{ $t("components.item.attachments_list.open_in_service", { service: getServiceDisplayName(attachment) }) }} </TooltipContent>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/components/Item/AttachmentsList.vue` around lines 105 - 107, The template currently hardcodes the service name "Paperless" in the i18n interpolation for TooltipContent (key "components.item.attachments_list.open_in_service"); replace this with a dynamic display name derived from the attachment's MIME type by calling a helper that looks up the adapter registry (e.g., create a helper like getServiceDisplayNameFromMime(mimeType) and use it in the component's computed properties or methods to pass the resolved service name into $t). Ensure TooltipContent uses the computed/serviceName value instead of the hardcoded string so future services are handled automatically.
395-444: ⚖️ Poor tradeoffSilent error swallowing in enrichment sub-fetches.
The correspondent, document_type, and tag fetches use
.catch(() => {})to silently swallow errors (lines 407, 423, 443). This is graceful degradation — if a related entity fetch fails, the main document data is still shown with partial enrichment.This is likely intentional, but consider whether users should be notified when partial data couldn't be loaded (e.g., "Some metadata unavailable"). Current behavior shows incomplete data without indication.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/components/Item/AttachmentsList.vue` around lines 395 - 444, The sub-fetches for correspondent, document_type, and tags in AttachmentsList.vue currently swallow errors with .catch(() => {}) which hides partial-enrichment failures; replace those empty catches by capturing the error (e) and (1) logging it (console.error or a logger) and (2) marking a shared flag (e.g., enrichmentFailed or push to enrichmentErrors) accessible alongside doc/tagResults/jobs so the UI can render a small “Some metadata unavailable” notice when any enrichmentFailed is true; update the code paths that push to jobs (the correspondent/document_type/tag fetch chains) to set this flag on error and ensure the component renders the notification when enrichmentErrors.length>0.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go`:
- Around line 104-109: Replace use of http.DefaultClient in the integration
proxy handler with a dedicated HTTP client that enforces a timeout and sane
transport settings: add a package-level variable (e.g., proxyHTTPClient)
configured with Timeout: 30*time.Second and a Transport that sets MaxIdleConns,
IdleConnTimeout, and DisableCompression as suggested, ensure the package imports
time, and update the call in the handler (where resp, err :=
http.DefaultClient.Do(req)) to use proxyHTTPClient.Do(req) instead.
- Around line 124-126: The handler currently calls io.Copy(w, resp.Body) without
bounds (see w.WriteHeader and io.Copy usage) which can allow huge upstream
responses; wrap resp.Body with io.LimitReader (or use io.CopyN) and enforce a
hard size limit (e.g. const maxRespSize = 10 << 20) when copying to w, detect if
the upstream exceeds that limit and return a safe error/status (e.g. write
http.StatusBadGateway or http.StatusRequestEntityTooLarge and a small error
body) instead of streaming unlimited data; update the code around the resp.Body
copy to use io.LimitReader(resp.Body, maxRespSize+1) and check if bytesCopied >
maxRespSize to handle the overflow case.
In `@backend/internal/core/services/service_items_attachments_external_test.go`:
- Around line 37-44: The test data in the knownSources variable still contains
an Immich entry even though Immich was removed; update the knownSources slice in
service_items_attachments_external_test.go by removing the {"immich",
"1df4f848-dead-beef-cafe-123456789abc"} element (or, if Immich is intentionally
kept, update the PR summary to state that Immich support remains) so the test
data matches the intended adapter registry state; edit the knownSources
declaration to only include the remaining valid sources (e.g., "paperless" and
"link") and run the tests.
In `@frontend/components/Item/AttachmentsList.vue`:
- Around line 351-363: The function describeRequestError returns hardcoded
English messages; replace them with i18n lookups using the
component/localization instance (e.g., $t) and add corresponding keys
(components.item.attachments_list.errors.auth_failed, .request_failed with
{status}, and .service_unreachable with {baseUrl}) to your locale files (e.g.,
frontend/locales/en.json); update describeRequestError to call $t(...) for the
401/403 branch, the generic HTTP branch (passing status), and the network-level
branch (passing baseUrl) so all user-facing strings are translatable.
In `@frontend/lib/integration-adapters.test.ts`:
- Around line 40-42: The extractPaperlessDocId implementation silently falls
back to pattern-only matching when parsing the configured baseUrl fails; update
the catch block in extractPaperlessDocId to emit a developer-facing warning
(e.g., console.warn or the app logger.warn) that includes the invalid baseUrl
and the parse error, and add a small validation helper (e.g.,
validatePaperlessBaseUrl) to run at configuration time to validate/normalize the
baseUrl and surface errors earlier instead of silently relying on heuristic
pattern matching.
In `@frontend/pages/item/`[id]/index/edit.vue:
- Around line 91-102: The loadIntegrationSettings function silently returns when
api.user.getSettings() fails or data?.item is missing; update
loadIntegrationSettings to handle errors by logging the error (e.g.,
console.error or process logger) and surface a user-facing notification/toast so
users know settings failed to load, while preserving the existing fallback to
empty integrationSettings; specifically catch or check for error from
api.user.getSettings(), log the error and call the component's
notification/toast method (or emit an event) before returning, and also validate
data.item before casting to Record<string, unknown> (refer to
loadIntegrationSettings, api.user.getSettings, and integrationSettings).
- Around line 414-419: Replace the hardcoded `${serviceName} linked` toast with
a translatable string: use the i18n key (e.g. "items.toast.service_linked") and
call t with a replacement object so the service name (from
classified.adapter.name → serviceName) is injected, then pass that translated
string into toast.success; ensure the locale files include the "{service}
linked" entry and update the toast invocation near the classified handling code
that computes serviceName.
- Around line 40-46: Remove the unused Immich fields from the reactive
integrationSettings object: delete immich_url and immich_token so
integrationSettings only contains paperless_url and paperless_token; also remove
any code that assigns loaded settings into integrationSettings. Leave
classifyDroppedUrl and SERVICE_ADAPTERS unchanged (they only use Paperless), and
ensure no other references to integrationSettings. This will eliminate vestigial
Immich config while keeping the Paperless integration intact.
---
Nitpick comments:
In `@backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go`:
- Around line 41-128: Add per-user rate limiting to the integration proxy by
wrapping the V1Controller.HandleIntegrationProxy handler with a user-scoped
limiter middleware: implement or reuse an integrationProxyLimiter that keys
limits by the authenticated user ID (services.UseUserCtx(ctx.Context).ID) and
attach its middleware when registering the route (e.g., in routes.go wrap the
GET "/integrations/{name}/proxy" with
chain.ToHandlerFunc(v1Ctrl.HandleIntegrationProxy(), append(userMW,
a.integrationProxyLimiter.middleware)...)); ensure the limiter runs before the
handler and returns a proper 429/Retry-After response when the per-user quota is
exceeded.
In `@frontend/components/Item/AttachmentsList.vue`:
- Around line 105-107: The template currently hardcodes the service name
"Paperless" in the i18n interpolation for TooltipContent (key
"components.item.attachments_list.open_in_service"); replace this with a dynamic
display name derived from the attachment's MIME type by calling a helper that
looks up the adapter registry (e.g., create a helper like
getServiceDisplayNameFromMime(mimeType) and use it in the component's computed
properties or methods to pass the resolved service name into $t). Ensure
TooltipContent uses the computed/serviceName value instead of the hardcoded
string so future services are handled automatically.
- Around line 395-444: The sub-fetches for correspondent, document_type, and
tags in AttachmentsList.vue currently swallow errors with .catch(() => {}) which
hides partial-enrichment failures; replace those empty catches by capturing the
error (e) and (1) logging it (console.error or a logger) and (2) marking a
shared flag (e.g., enrichmentFailed or push to enrichmentErrors) accessible
alongside doc/tagResults/jobs so the UI can render a small “Some metadata
unavailable” notice when any enrichmentFailed is true; update the code paths
that push to jobs (the correspondent/document_type/tag fetch chains) to set this
flag on error and ensure the component renders the notification when
enrichmentErrors.length>0.
In `@frontend/composables/preferences-utils.ts`:
- Around line 89-101: mergeSyncedSettings currently silences TypeScript by
casting settings[key] to never before assigning to nextPreferences; instead make
the assignment type-safe by either (A) narrowing the settings parameter to a
keyed type that maps known preference keys to their allowed value types (e.g.,
Record<keyof LocationViewPreferences, ...>) so you can assign without
assertions, or (B) perform a runtime type-check per key inside
mergeSyncedSettings using a small type-check map/type guard for each preference
key (validate settings[key] matches the expected type) and only then assign to
nextPreferences; remove the `as never` cast and replace it with the validated
value (or tighten the function signature) and keep forEachSyncedPreference usage
the same.
In `@frontend/composables/use-preferences.test.ts`:
- Around line 38-43: The test uses an invalid cast "theme: \"dark\" as never"
which hides type errors; update the test in use-preferences.test.ts to supply a
valid DaisyTheme or remove the cast: change the prefs object (currently spread
from DEFAULT_PREFERENCES) passed to buildSyncedSettings(SYNC_ALL) so theme is
either a real DaisyTheme value (e.g., "homebox" or whatever is defined in the
DaisyTheme union) or, if "dark" is valid, drop the `as never` and use `theme:
"dark"`. Ensure consistency with the later test that already uses `theme:
"dark"` without a cast and keep references to DEFAULT_PREFERENCES,
buildSyncedSettings, and SYNC_ALL unchanged.
In `@frontend/pages/profile.vue`:
- Around line 515-520: Replace the current FormTextField usage for the API token
with a field that supports visibility toggling (either swap to the existing
FormPassword component or add the same toggle behavior) so users can show/hide
integrationSettings.paperlessToken when verifying input; update the component at
the FormTextField instance in profile.vue (the block rendering
integrationSettings.paperlessToken with label profile.paperless_token and
placeholder profile.paperless_token_placeholder) to use FormPassword or mirror
its show/hide logic and props.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3a8cf4a1-03dd-4a60-8ea5-c76e7c97c8d2
⛔ Files ignored due to path filters (1)
backend/go.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
backend/app/api/handlers/v1/v1_ctrl_integration_proxy.gobackend/app/api/routes.gobackend/internal/core/services/service_items_attachments_external_test.gobackend/internal/data/repo/repo_item_attachments.gobackend/internal/data/repo/repo_item_attachments_test.godocs/todo-integration-features.mdfrontend/components/Item/AttachmentsList.vuefrontend/composables/preferences-utils.tsfrontend/composables/use-preferences.test.tsfrontend/composables/use-preferences.tsfrontend/lib/integration-adapters.test.tsfrontend/lib/integration-adapters.tsfrontend/locales/en.jsonfrontend/pages/item/[id]/index/edit.vuefrontend/pages/profile.vuefrontend/stores/integration-cache.ts
szaiser
pushed a commit
to szaiser/homebox
that referenced
this pull request
May 12, 2026
…dia#1492 - backend: replace http.DefaultClient with dedicated proxyHTTPClient (30s timeout, bounded transport) to prevent upstream hangs (critical) - backend: cap proxy response at 10 MB via io.LimitReader to prevent memory exhaustion from unbounded upstream responses (major) - frontend: remove vestigial immich_url / immich_token fields from integrationSettings in edit.vue; these were never used after the Immich removal (minor) - frontend: add console.warn in loadIntegrationSettings on API failure instead of silently returning (minor) - frontend: replace hardcoded service-linked toast string with i18n key items.toast.service_linked (minor) - frontend: replace hardcoded English strings in describeRequestError with i18n keys under components.item.attachments_list.errors (minor) - frontend: add console.warn in integration-adapters.ts catch block when baseUrl fails to parse (minor) - locales: add items.toast.service_linked and components.item.attachments_list.errors.{auth_failed,request_failed, service_unreachable} keys to en.json
szaiser
pushed a commit
to szaiser/homebox
that referenced
this pull request
May 12, 2026
- Remove MimeTypeImmichAsset constant from repo_item_attachments.go - Remove immich from externalLinkMimeTypes slice - Remove immich case from MimeTypeForSourceType() - Remove immich test block from repo_item_attachments_test.go (TestMimeTypeForSourceType and externalLinkMimeTypeCases) - Remove immich entry from knownSources in service_items_attachments_external_test.go Resolves CodeRabbit review comment on PR sysadminsmedia#1492 (immich inconsistency).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go (1)
35-52: ⚡ Quick winUpdate the godoc or implement per-integration auth scheme support.
Line 114 hardcodes
Authorization: Token <token>for all integrations. This contradicts the godoc claim on lines 41–43 that "adding a new integration only requires a Vue component and a settings entry — no new Go code." Paperless usesTokenauth, but Immich usesx-api-key, and most modern APIs useAuthorization: Bearer. The frontend adapter registry is already extensible, but the backend lacks a way to specify per-integration auth schemes.Either:
- Store auth scheme in settings (
{name}_auth_scheme, default"Token"), or- Add a backend per-integration registry mapping names to auth scheme handlers
Choose one to make the endpoint's actual behavior match its documented behavior, especially important for multi-tenancy/user data isolation considerations with external service credentials.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go` around lines 35 - 52, The handler HandleIntegrationProxy currently always sets "Authorization: Token <token>" using the {name}_token setting; update it to read an optional {name}_auth_scheme setting (default "Token") and use that to build the outgoing auth header (support at least "Token", "Bearer", and "x-api-key" schemes) before sending the proxied request, so the code that retrieves {name}_token and sets the header is replaced with logic that switches on authScheme and sets either Authorization: Token <token>, Authorization: Bearer <token>, or X-API-Key: <token> accordingly; also update the godoc to reflect that integrations may specify an auth scheme via settings and/or validate unknown schemes with a 400 or fall back to default.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go`:
- Around line 138-144: Replace the current truncation check that treats n ==
maxResponseSize as truncated by reading one extra byte: use
io.LimitReader(resp.Body, maxResponseSize+1) (instead of maxResponseSize) when
copying to w, then treat truncation only when n > maxResponseSize; keep logging
via log.Warn().Str("integration", name).Int64("bytes", n).Msg(...) and preserve
the existing copyErr handling. This ensures a true positive truncation detection
for the variables n, copyErr, resp.Body and maxResponseSize in the integration
proxy response copy block.
- Around line 92-108: The handler currently proxies user-provided
settings[name+"_url"] without IP filtering, hardcodes the auth scheme as "Token
"+token, and misdetects truncation; fix by (1) adding a custom DialContext to
proxyHTTPClient that resolves the target hostname and rejects connections to
loopback, link-local, unspecified and RFC1918/private ranges (or enforce
HTTPS/host allow-list) before dialing; reference proxyHTTPClient and the
upstream variable to locate where to apply the DialContext and validation; (2)
make the auth scheme configurable by reading settings[name+"_auth_scheme"]
(fallback to "Token") instead of always using "Token "+token so integrations can
use Bearer/Basic/x-api-key; reference the code that sets the Authorization
header; and (3) detect response truncation correctly by reading with
io.LimitReader(resp.Body, maxResponseSize+1) and checking if bytesRead >
maxResponseSize (reference maxResponseSize and the io.LimitReader usage).
In `@frontend/pages/item/`[id]/index/edit.vue:
- Around line 89-99: The loadIntegrationSettings function should avoid logging
the raw error object and must validate setting types before assigning to
integrationSettings; change the error logging to a generic message (e.g.,
"Failed to load integration settings") and, if you need details, log only safe
fields like error?.message, and ensure api.user.getSettings() errors are not
printing full payloads. For assignments to integrationSettings.paperless_url and
paperless_token, check the type of settings.paperless_url and
settings.paperless_token (e.g., typeof === "string") before using them,
otherwise assign an empty string or a safe default; also treat data.item
defensively (ensure it's an object) when reading values from
api.user.getSettings.
---
Nitpick comments:
In `@backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go`:
- Around line 35-52: The handler HandleIntegrationProxy currently always sets
"Authorization: Token <token>" using the {name}_token setting; update it to read
an optional {name}_auth_scheme setting (default "Token") and use that to build
the outgoing auth header (support at least "Token", "Bearer", and "x-api-key"
schemes) before sending the proxied request, so the code that retrieves
{name}_token and sets the header is replaced with logic that switches on
authScheme and sets either Authorization: Token <token>, Authorization: Bearer
<token>, or X-API-Key: <token> accordingly; also update the godoc to reflect
that integrations may specify an auth scheme via settings and/or validate
unknown schemes with a 400 or fall back to default.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b20defff-c331-4d61-90b5-90df83d23fbb
📒 Files selected for processing (5)
backend/app/api/handlers/v1/v1_ctrl_integration_proxy.gofrontend/components/Item/AttachmentsList.vuefrontend/lib/integration-adapters.tsfrontend/locales/en.jsonfrontend/pages/item/[id]/index/edit.vue
✅ Files skipped from review due to trivial changes (1)
- frontend/locales/en.json
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/lib/integration-adapters.ts
- frontend/components/Item/AttachmentsList.vue
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/internal/data/repo/repo_item_attachments.go (1)
98-110: Ensure path validation occurs at the API handler layer.The repository correctly stores the external ID without validation (appropriate for the data access layer). Per the PR objectives, ensure the API handler validates both the
nameandpathparameters before calling repository methods to prevent injection or traversal attacks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/data/repo/repo_item_attachments.go` around lines 98 - 110, The repo currently accepts external IDs without validation (e.g., MimeTypeForSourceType in repo_item_attachments.go), so add explicit validation in the API handler(s) that call the repository attachment methods: check both name and path are non-empty, within length limits, match an allowed character set (or whitelist), normalize with filepath.Clean and then reject values that are absolute (start with "/") or contain path traversal (cleaned starts with ".." or contains ".." segments or differs from a safe relative representation), and return a 4xx error before invoking any repo methods; implement this validation in the handler(s) that parse the incoming request and call the repo attachment functions so the data layer continues to accept raw IDs but only after the API layer has validated them.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@backend/internal/data/repo/repo_item_attachments.go`:
- Around line 98-110: The repo currently accepts external IDs without validation
(e.g., MimeTypeForSourceType in repo_item_attachments.go), so add explicit
validation in the API handler(s) that call the repository attachment methods:
check both name and path are non-empty, within length limits, match an allowed
character set (or whitelist), normalize with filepath.Clean and then reject
values that are absolute (start with "/") or contain path traversal (cleaned
starts with ".." or contains ".." segments or differs from a safe relative
representation), and return a 4xx error before invoking any repo methods;
implement this validation in the handler(s) that parse the incoming request and
call the repo attachment functions so the data layer continues to accept raw IDs
but only after the API layer has validated them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9e9b5dc7-a03c-42fb-afb7-809396468b4a
📒 Files selected for processing (4)
backend/internal/core/services/service_items_attachments_external_test.gobackend/internal/data/repo/repo_item_attachments.gobackend/internal/data/repo/repo_item_attachments_test.gofrontend/composables/use-preferences.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/composables/use-preferences.test.ts
- backend/internal/core/services/service_items_attachments_external_test.go
szaiser
pushed a commit
to szaiser/homebox
that referenced
this pull request
May 12, 2026
- Add ssrfSafeDialContext: custom DialContext that rejects loopback, link-local (incl. AWS/GCP/Azure IMDS 169.254.169.254), RFC1918, shared (RFC6598), and ULA ranges; public hostnames are unrestricted. Both literal IPs and DNS-resolved names are validated before dialing. - Add blockedCIDRs package-level var (lazy-initialised via IIFE). - Add checkBlockedIP helper. - Wire ssrfSafeDialContext into proxyHTTPClient Transport.DialContext. - Validate baseURL scheme (http:// or https://) before proxying. - Fix truncation false-positive: buffer with LimitReader(body, max+1), detect len(buf) > max and return 502 before writing any headers, so callers never receive a partial 200 with invalid JSON. - Reject known-oversized responses via Content-Length check upfront. - Fix loadIntegrationSettings (edit.vue): drop raw error from console.warn; use typeof guards instead of unsafe string casts. Resolves CodeRabbit review comments on PR sysadminsmedia#1492.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go`:
- Around line 194-200: The outbound request is created with http.NewRequest
which loses the incoming request's context; replace it with
http.NewRequestWithContext(req.Context(), http.MethodGet, upstream, nil) (or the
handler's ctx variable) so client cancellations and deadlines propagate
upstream, then continue to set the Authorization header and call
proxyHTTPClient.Do(req) as before and preserve the existing error handling via
validate.NewRequestError.
- Around line 200-203: The upstream URL is logged verbatim in the error path,
which can leak query strings or credentials; before calling log.Err(...) replace
the raw upstream with a sanitized version (e.g., parse upstream with net/url,
clear u.User and u.RawQuery/Fragment, then build a string like u.Scheme + "://"
+ u.Host + u.Path or simply u.Host + u.Path) and use that sanitized value in the
.Str("upstream", ...) field; implement this directly where
proxyHTTPClient.Do(req) error is handled (the log.Err(err).Str("integration",
name).Str("upstream", upstream).Msg(...) call) or extract the logic into a small
helper function (e.g., sanitizeUpstreamURL) and call it before logging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0dfd9b0a-6960-425c-9d79-69e51c6182ba
📒 Files selected for processing (2)
backend/app/api/handlers/v1/v1_ctrl_integration_proxy.gofrontend/pages/item/[id]/index/edit.vue
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/pages/item/[id]/index/edit.vue
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/internal/core/services/service_items_attachments_external_test.go (1)
149-150: ⚡ Quick winAssert no persistence on rejected source type (security contract).
This test checks the error path, but it does not verify the “before any DB write” guarantee. Add a post-check that the entity still has zero attachments after the call to lock in the allowlist/no-write behavior for untrusted
sourceTypeinput.Suggested test hardening
func TestEntityService_AttachmentAddExternalLink_UnknownSourceType(t *testing.T) { svc := &EntityService{repo: tRepos} entity := newExternalLinkEntity(t) _, err := svc.AttachmentAddExternalLink(tCtx, entity.ID, "unknown-source", "42", "Unknown", attachment.TypeAttachment) assert.Error(t, err) + + latest, getErr := svc.repo.Entities.GetOneByGroup(tCtx, tCtx.GID, entity.ID) + require.NoError(t, getErr) + assert.Empty(t, latest.Attachments) }As per coding guidelines, service-layer behavior should be validated in service tests; this strengthens the rejection-path contract in
backend/internal/core/services/.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/core/services/service_items_attachments_external_test.go` around lines 149 - 150, The test calls svc.AttachmentAddExternalLink(tCtx, entity.ID, "unknown-source", "42", "Unknown", attachment.TypeAttachment) and asserts an error but doesn't verify no DB write occurred; after the call add a retrieval of the entity via the same service/repo path (e.g., call svc.Get or svc.EntityByID used elsewhere) or query the attachments for entity.ID and assert length is zero to ensure no attachment was persisted when sourceType is untrusted, preserving the “no-write on rejected source” contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@backend/internal/core/services/service_items_attachments_external_test.go`:
- Around line 149-150: The test calls svc.AttachmentAddExternalLink(tCtx,
entity.ID, "unknown-source", "42", "Unknown", attachment.TypeAttachment) and
asserts an error but doesn't verify no DB write occurred; after the call add a
retrieval of the entity via the same service/repo path (e.g., call svc.Get or
svc.EntityByID used elsewhere) or query the attachments for entity.ID and assert
length is zero to ensure no attachment was persisted when sourceType is
untrusted, preserving the “no-write on rejected source” contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 16c4eef5-1245-416f-8352-e998fd3ca406
📒 Files selected for processing (11)
backend/app/api/handlers/v1/v1_ctrl_integration_proxy.gobackend/internal/core/services/service_items_attachments_external_test.gobackend/internal/data/repo/repo_item_attachments.gobackend/internal/data/repo/repo_item_attachments_test.gofrontend/components/Item/AttachmentsList.vuefrontend/composables/use-preferences.test.tsfrontend/lib/integration-adapters.test.tsfrontend/lib/integration-adapters.tsfrontend/pages/item/[id]/index/edit.vuefrontend/pages/profile.vuefrontend/stores/integration-cache.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- frontend/pages/profile.vue
- backend/internal/data/repo/repo_item_attachments.go
- frontend/stores/integration-cache.ts
- frontend/composables/use-preferences.test.ts
- frontend/lib/integration-adapters.ts
- backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go
- frontend/components/Item/AttachmentsList.vue
- backend/internal/data/repo/repo_item_attachments_test.go
Contributor
Author
|
@tankerkiller125 From my perspective, the PR is ready for review. Will you find time to review anytime soon? Let me know if there is any way to assist (refactor, simplify, split, share info, ..) |
szaiser
pushed a commit
to szaiser/homebox
that referenced
this pull request
May 16, 2026
…dia#1492 - backend: replace http.DefaultClient with dedicated proxyHTTPClient (30s timeout, bounded transport) to prevent upstream hangs (critical) - backend: cap proxy response at 10 MB via io.LimitReader to prevent memory exhaustion from unbounded upstream responses (major) - frontend: remove vestigial immich_url / immich_token fields from integrationSettings in edit.vue; these were never used after the Immich removal (minor) - frontend: add console.warn in loadIntegrationSettings on API failure instead of silently returning (minor) - frontend: replace hardcoded service-linked toast string with i18n key items.toast.service_linked (minor) - frontend: replace hardcoded English strings in describeRequestError with i18n keys under components.item.attachments_list.errors (minor) - frontend: add console.warn in integration-adapters.ts catch block when baseUrl fails to parse (minor) - locales: add items.toast.service_linked and components.item.attachments_list.errors.{auth_failed,request_failed, service_unreachable} keys to en.json
szaiser
pushed a commit
to szaiser/homebox
that referenced
this pull request
May 16, 2026
- Remove MimeTypeImmichAsset constant from repo_item_attachments.go - Remove immich from externalLinkMimeTypes slice - Remove immich case from MimeTypeForSourceType() - Remove immich test block from repo_item_attachments_test.go (TestMimeTypeForSourceType and externalLinkMimeTypeCases) - Remove immich entry from knownSources in service_items_attachments_external_test.go Resolves CodeRabbit review comment on PR sysadminsmedia#1492 (immich inconsistency).
szaiser
pushed a commit
to szaiser/homebox
that referenced
this pull request
May 16, 2026
- Add ssrfSafeDialContext: custom DialContext that rejects loopback, link-local (incl. AWS/GCP/Azure IMDS 169.254.169.254), RFC1918, shared (RFC6598), and ULA ranges; public hostnames are unrestricted. Both literal IPs and DNS-resolved names are validated before dialing. - Add blockedCIDRs package-level var (lazy-initialised via IIFE). - Add checkBlockedIP helper. - Wire ssrfSafeDialContext into proxyHTTPClient Transport.DialContext. - Validate baseURL scheme (http:// or https://) before proxying. - Fix truncation false-positive: buffer with LimitReader(body, max+1), detect len(buf) > max and return 502 before writing any headers, so callers never receive a partial 200 with invalid JSON. - Reject known-oversized responses via Content-Length check upfront. - Fix loadIntegrationSettings (edit.vue): drop raw error from console.warn; use typeof guards instead of unsafe string casts. Resolves CodeRabbit review comments on PR sysadminsmedia#1492.
szaiser
force-pushed
the
feat/paperless-immich-attachement-links
branch
from
1ed1bc6 to
30e18e2
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/pages/item/`[id]/index/edit.vue:
- Line 835: The external-link gate currently checks attachment.path with
case/whitespace-sensitive startsWith checks; replace that predicate with the
existing URL validator by calling isValidHttpURL on the attachment path (trimmed
and handling null/undefined) so the Tooltip v-if uses
isValidHttpURL(attachment.path ?? '') (or isValidHttpURL((attachment.path ??
'').trim())) instead of the startsWith checks; update the Tooltip condition and
any related external-link action gating to reuse isValidHttpURL to ensure
consistent, case-insensitive, whitespace-tolerant validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e8ee0971-91ac-4090-806c-42d93bb3d4b2
⛔ Files ignored due to path filters (1)
backend/go.sumis excluded by!**/*.sum
📒 Files selected for processing (1)
frontend/pages/item/[id]/index/edit.vue
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/components/Location/CreateModal.vue (1)
8-15:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHardcoded strings should be translatable.
Lines 8 and 15 contain hardcoded UI strings ("Type" and "Select type...") that should use
$t()for internationalization, consistent with the rest of the component.🌐 Proposed fix
- <Label for="location-type-select" class="px-1">Type</Label> + <Label for="location-type-select" class="px-1">{{ $t('global.type') }}</Label> <select id="location-type-select" class="w-full rounded-md border bg-background px-3 py-2 text-sm" :value="selectedEntityType?.id || ''" `@change`="onEntityTypeChanged(($event.target as HTMLSelectElement).value)" > - <option value="">Select type...</option> + <option value="">{{ $t('global.select') }}</option>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/components/Location/CreateModal.vue` around lines 8 - 15, Replace the two hardcoded UI strings with translatable calls: change the Label text "Type" to use $t(...) and change the option "Select type..." to use $t(...), e.g., $t('location.type') and $t('location.selectType') (or your project's established keys), keeping the same elements (Label, select option) and leaving the :value and `@change` handler (onEntityTypeChanged, selectedEntityType) intact so behavior doesn't change.Source: Coding guidelines
🧹 Nitpick comments (2)
frontend/components/Template/CreateModal.vue (1)
147-159: 💤 Low valueConsider extracting the
newTemplateField()factory to a shared utility.The same factory function with identical logic exists in
frontend/pages/template/[id].vue(lines 87-98). While this duplication is minor and localized, extracting it to a shared utility would improve maintainability and ensure consistency if the default field shape changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/components/Template/CreateModal.vue` around lines 147 - 159, Extract the duplicated factory by moving DEFAULT_TIME_VALUE and the newTemplateField() function (which returns a TemplateField with id: NIL_UUID, name:"", type:"text", booleanValue:false, numberValue:0, textValue:"", timeValue:DEFAULT_TIME_VALUE) into a shared utility module (e.g., a new file under frontend/utils or frontend/lib) and export newTemplateField; then replace the local implementations in CreateModal.vue and pages/template/[id].vue with an import of the shared newTemplateField and remove the duplicate definitions, ensuring NIL_UUID is referenced from the same source or re-exported if needed so both components use the single shared factory.backend/app/api/main.go (1)
203-216: middleware.RealIP removal: no breakage found for logging/security; confirmTrustProxyconfig for rate limiting
mid.Logger(backend/internal/web/mid/logger.go) logs only method/path/status/rid and does not readr.RemoteAddr, so request logging won’t be affected by droppingmiddleware.RealIP.- Rate limiting uses
extractClientIP(..., trustProxy)(backend/app/api/middleware.go), which trustsX-Real-IP/X-Forwarded-Foronly whentrustProxyis enabled, otherwise it falls back tor.RemoteAddr.- Codebase search shows
r.RemoteAddrusage in handlers is limited to the limiter/IP-extraction path (no other request/security logic appears to depend onr.RemoteAddr).Security recommendation: ensure
cfg.AuthRateLimit.Options.TrustProxyis set appropriately for your deployment (behind a proxy/load balancer, otherwise rate-limit keys may become the proxy IP).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/api/main.go` around lines 203 - 216, Removed middleware.RealIP may change how client IPs are seen by rate limiting; confirm and ensure TrustProxy is configured correctly. Verify that cfg.AuthRateLimit.Options.TrustProxy is set for deployments behind a proxy/load-balancer so extractClientIP (used by the rate limiter in middleware.go) will trust X-Forwarded-For/X-Real-IP when appropriate; if not behind a proxy leave TrustProxy false so extractClientIP falls back to r.RemoteAddr. Also review mid.Logger and other middleware (mid.Logger, middleware.Recoverer) to ensure no other code expects middleware.RealIP and update configuration or docs to state the required TrustProxy setting for correct rate-limiting behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/internal/core/services/main_test.go`:
- Around line 61-63: The test spawns the event bus with context.Background() and
ignores its error, leaking the goroutine and hiding failures; change to use a
cancellable context (ctx, cancel := context.WithCancel(context.Background()))
and register cancel via t.Cleanup(cancel) so the bus is stopped when the test
ends, run tbus.Run(ctx) in a goroutine but send its returned error on a channel,
and in t.Cleanup wait for that goroutine to exit and fail the test (t.Fatalf or
t.Error) if the run returned a non-nil error so panics/failures aren’t silently
discarded; reference tbus.Run and the anonymous goroutine when making these
changes.
In `@backend/internal/data/repo/repo_entity_types.go`:
- Around line 85-90: Remove the calls to GetDefault from the GetAll method so
GetAll remains a pure read operation: delete the two GetDefault(ctx, gid, ...)
invocations in repo_entity_types.go and revert GetAll to only query and return
existing entity types. Move default-creation responsibility to the group
initialization flow (e.g., add a call from GroupRepository.Create or an explicit
EnsureDefaults(ctx, groupID) invoked during group creation) so defaults are
created once inside a transactional setup path instead of lazily in GetAll;
ensure EnsureDefaults (or the group create flow) calls the same GetDefault logic
and handles errors/concurrency there.
---
Outside diff comments:
In `@frontend/components/Location/CreateModal.vue`:
- Around line 8-15: Replace the two hardcoded UI strings with translatable
calls: change the Label text "Type" to use $t(...) and change the option "Select
type..." to use $t(...), e.g., $t('location.type') and $t('location.selectType')
(or your project's established keys), keeping the same elements (Label, select
option) and leaving the :value and `@change` handler (onEntityTypeChanged,
selectedEntityType) intact so behavior doesn't change.
---
Nitpick comments:
In `@backend/app/api/main.go`:
- Around line 203-216: Removed middleware.RealIP may change how client IPs are
seen by rate limiting; confirm and ensure TrustProxy is configured correctly.
Verify that cfg.AuthRateLimit.Options.TrustProxy is set for deployments behind a
proxy/load-balancer so extractClientIP (used by the rate limiter in
middleware.go) will trust X-Forwarded-For/X-Real-IP when appropriate; if not
behind a proxy leave TrustProxy false so extractClientIP falls back to
r.RemoteAddr. Also review mid.Logger and other middleware (mid.Logger,
middleware.Recoverer) to ensure no other code expects middleware.RealIP and
update configuration or docs to state the required TrustProxy setting for
correct rate-limiting behavior.
In `@frontend/components/Template/CreateModal.vue`:
- Around line 147-159: Extract the duplicated factory by moving
DEFAULT_TIME_VALUE and the newTemplateField() function (which returns a
TemplateField with id: NIL_UUID, name:"", type:"text", booleanValue:false,
numberValue:0, textValue:"", timeValue:DEFAULT_TIME_VALUE) into a shared utility
module (e.g., a new file under frontend/utils or frontend/lib) and export
newTemplateField; then replace the local implementations in CreateModal.vue and
pages/template/[id].vue with an import of the shared newTemplateField and remove
the duplicate definitions, ensuring NIL_UUID is referenced from the same source
or re-exported if needed so both components use the single shared factory.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d595a077-6aa9-4182-86e9-7240ec1b5a55
⛔ Files ignored due to path filters (2)
backend/go.sumis excluded by!**/*.sumfrontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (38)
backend/app/api/main.gobackend/app/api/routes.gobackend/internal/core/services/main_test.gobackend/internal/core/services/service_items_attachments_external_test.gobackend/internal/data/repo/repo_entity_templates.gobackend/internal/data/repo/repo_entity_types.gofrontend/components/Item/AttachmentsList.vuefrontend/components/Item/Card.vuefrontend/components/Item/CreateModal.vuefrontend/components/Item/View/ItemChangeDetails.vuefrontend/components/Item/View/table/columns.tsfrontend/components/Item/View/table/data-table-expanded-row.vuefrontend/components/Location/CreateModal.vuefrontend/components/Scanner/AROverlayCard.vuefrontend/components/Template/Card.vuefrontend/components/Template/CreateModal.vuefrontend/composables/use-barcode-detector.tsfrontend/composables/use-preferences.tsfrontend/lib/api/__test__/factories/index.tsfrontend/lib/api/__test__/test-utils.tsfrontend/lib/api/__test__/user/items.test.tsfrontend/lib/api/__test__/user/tags.test.tsfrontend/lib/api/__test__/user/templates.test.tsfrontend/lib/api/classes/items.tsfrontend/lib/integration-adapters.tsfrontend/locales/en.jsonfrontend/package.jsonfrontend/pages/collection/index/entity-types.vuefrontend/pages/collection/index/tools.vuefrontend/pages/item/[id]/index.vuefrontend/pages/item/[id]/index/edit.vuefrontend/pages/location/[id]/index/edit.vuefrontend/pages/location/[id]/index/index.vuefrontend/pages/profile.vuefrontend/pages/reports/label-generator.vuefrontend/pages/reset-password.vuefrontend/pages/template/[id].vuefrontend/stores/integration-cache.ts
💤 Files with no reviewable changes (13)
- frontend/pages/reset-password.vue
- frontend/pages/collection/index/tools.vue
- frontend/package.json
- frontend/pages/item/[id]/index.vue
- frontend/pages/reports/label-generator.vue
- frontend/pages/collection/index/entity-types.vue
- frontend/pages/location/[id]/index/index.vue
- frontend/pages/template/[id].vue
- frontend/pages/item/[id]/index/edit.vue
- frontend/pages/location/[id]/index/edit.vue
- frontend/stores/integration-cache.ts
- frontend/pages/profile.vue
- frontend/lib/integration-adapters.ts
✅ Files skipped from review due to trivial changes (2)
- frontend/lib/api/classes/items.ts
- frontend/locales/en.json
🚧 Files skipped from review as they are similar to previous changes (4)
- backend/app/api/routes.go
- frontend/composables/use-preferences.ts
- backend/internal/core/services/service_items_attachments_external_test.go
- frontend/components/Item/AttachmentsList.vue
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/pages/template/[id].vue (1)
107-111:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve existing field type in edit mapping.
Line 110 hardcodes
typeto"text", which rewrites persisted field types when saving updates and can corrupt template field semantics.Proposed fix
fields: template.value.fields.map(f => ({ id: f.id, name: f.name, - type: "text" as const, + type: f.type, booleanValue: f.booleanValue, numberValue: f.numberValue, textValue: f.textValue, timeValue: f.timeValue, })),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/pages/template/`[id].vue around lines 107 - 111, The mapping in template.value.fields.map currently hardcodes the field type ("type": "text") which overwrites persisted types; change the mapper to preserve the original type by using the existing property (e.g., use f.type) when building each field object in the map so Template editing does not corrupt field semantics (update the mapping in the code that constructs fields from template.value.fields).
🧹 Nitpick comments (1)
backend/internal/data/repo/repo_entity_types_test.go (1)
22-41: ⚡ Quick winAdd an idempotency assertion for
EnsureDefaults.Call
EnsureDefaultstwice and assert the list remains exactly 2 entries. This locks the intended bootstrap contract against duplicate-default regressions.♻️ Suggested test addition
func TestEntityTypeRepository_EnsureDefaults(t *testing.T) { ctx := context.Background() group, err := tRepos.Groups.GroupCreate(ctx, "entity-types-defaults-"+uuid.NewString(), uuid.Nil) require.NoError(t, err) require.NoError(t, tRepos.EntityTypes.EnsureDefaults(ctx, group.ID)) + require.NoError(t, tRepos.EntityTypes.EnsureDefaults(ctx, group.ID)) entityTypes, err := tRepos.EntityTypes.GetAll(ctx, group.ID) require.NoError(t, err) require.Len(t, entityTypes, 2)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/data/repo/repo_entity_types_test.go` around lines 22 - 41, Add an idempotency check to TestEntityTypeRepository_EnsureDefaults: after the first call to tRepos.EntityTypes.EnsureDefaults(ctx, group.ID) and the existing assertions, call tRepos.EntityTypes.EnsureDefaults(ctx, group.ID) a second time, then call tRepos.EntityTypes.GetAll(ctx, group.ID) again and assert the returned slice still has length 2 and contains the same "Item" (IsLocation false) and "Location" (IsLocation true) entries; this verifies EnsureDefaults is safe to call multiple times without creating duplicate entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/internal/core/services/service_group.go`:
- Around line 37-46: The group is being persisted with
svc.repos.Groups.GroupCreate before EnsureDefaults runs, so failures in
svc.repos.EntityTypes.EnsureDefaults can leave a created group; wrap both
operations in a single atomic transaction (or, if transactions are unavailable,
delete/rollback the created group on EnsureDefaults error). Specifically, run
GroupCreate and EnsureDefaults within the same DB transaction (or call
svc.repos.Groups.Delete/Remove using group.ID on EnsureDefaults failure), using
ctx.Context and group.ID to locate the created record and ensure proper
commit/rollback so either both succeed or no group remains.
In `@backend/internal/core/services/service_user.go`:
- Around line 562-563: The log currently includes the raw OIDC subject
(log.Debug().Str("issuer", issuer).Str("subject", subject)... and the similar
log at the other location); replace the raw subject with a non-PII-safe value by
computing and logging a deterministic hash (e.g., SHA-256) or a redacted flag
and change the field name (e.g., "subject_hash" or "subject_present") so you
never emit the plain subject; update both occurrences (the Debug call that
includes subject and the other log at the second occurrence) to use the
hashed/redacted value and keep issuer logging unchanged.
- Around line 198-205: Ensure that failures from
svc.repos.EntityTypes.EnsureDefaults do not cause the API to return an error
after the user/group has already been persisted: in both places where
EnsureDefaults is called (the bootstrap paths around bootstrapSpan/span and the
second path around lines 562-568), catch the error, call recordServiceSpanError
to record it and log the failure (using log.Error or process logger), but do NOT
return repo.UserOut{} with the error; instead proceed to return the successful
UserOut response and schedule/trigger a background retry or cleanup task for
EnsureDefaults so the client sees success while the service repairs the
incomplete defaults asynchronously. Ensure you update both call sites (the calls
to svc.repos.EntityTypes.EnsureDefaults, bootstrapSpan, span, and
recordServiceSpanError) to follow this pattern.
In `@frontend/components/Location/CreateModal.vue`:
- Around line 69-70: The create payload currently drops selected tags by
hardcoding tagIds: [] — use the TagSelector-bound form.tags instead: in the
payload construction (where tagIds is set) replace the hardcoded [] with the IDs
from form.tags (e.g. tagIds: form.tags.map(t => t.id) or tagIds: form.tags if
tags are primitives). Update the submit/create handler (the function that builds
the payload for location creation in CreateModal.vue) to pull from form.tags so
user-selected or template-default tags are sent; ensure form.tags is initialized
consistently where the form is created.
In `@frontend/lib/api/__test__/user/stats.test.ts`:
- Around line 43-45: The test builds a random set of locations but HB.location
is still chosen randomly which makes totalLocations flaky; change the assignment
of HB.location to deterministically pick from the generated locations array
(locations) instead of a random choice—e.g., derive an index from a stable value
like the test iteration or suffix and assign HB.location =
locations[stableIndex] so all three generated locations are always represented
for the totalLocations assertion; update the test code that references suffix,
tags, locations and HB.location accordingly.
---
Outside diff comments:
In `@frontend/pages/template/`[id].vue:
- Around line 107-111: The mapping in template.value.fields.map currently
hardcodes the field type ("type": "text") which overwrites persisted types;
change the mapper to preserve the original type by using the existing property
(e.g., use f.type) when building each field object in the map so Template
editing does not corrupt field semantics (update the mapping in the code that
constructs fields from template.value.fields).
---
Nitpick comments:
In `@backend/internal/data/repo/repo_entity_types_test.go`:
- Around line 22-41: Add an idempotency check to
TestEntityTypeRepository_EnsureDefaults: after the first call to
tRepos.EntityTypes.EnsureDefaults(ctx, group.ID) and the existing assertions,
call tRepos.EntityTypes.EnsureDefaults(ctx, group.ID) a second time, then call
tRepos.EntityTypes.GetAll(ctx, group.ID) again and assert the returned slice
still has length 2 and contains the same "Item" (IsLocation false) and
"Location" (IsLocation true) entries; this verifies EnsureDefaults is safe to
call multiple times without creating duplicate entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2f5fdbb2-8a27-455d-a418-2537e67e55ea
📒 Files selected for processing (13)
backend/internal/core/services/main_test.gobackend/internal/core/services/service_group.gobackend/internal/core/services/service_user.gobackend/internal/data/repo/main_test.gobackend/internal/data/repo/repo_entity_types.gobackend/internal/data/repo/repo_entity_types_test.gofrontend/components/Location/CreateModal.vuefrontend/components/Template/Card.vuefrontend/components/Template/CreateModal.vuefrontend/lib/api/__test__/user/stats.test.tsfrontend/lib/template-fields.tsfrontend/pages/item/[id]/index.vuefrontend/pages/template/[id].vue
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/components/Template/Card.vue
- frontend/pages/item/[id]/index.vue
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Action performedReviews resumed. |
Contributor
Author
|
@coderabbitai resume |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/components/Item/View/table/columns.ts (1)
156-163: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGuard
item.parentwithentityType?.isLocationbefore rendering the link.parentcan be a non-location entity, so/location/${loc.id}can point to the wrong page when the item is nested under another item.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/components/Item/View/table/columns.ts` around lines 156 - 163, The link rendering in the table cell currently assumes item.parent is always a location, which can send users to the wrong page. Update the cell renderer in columns.ts to only render the /location/${loc.id} anchor when entityType?.isLocation is true and item.parent exists; otherwise fall back to the muted placeholder. Use the existing row.original EntitySummary, item.parent, and entityType check in this column definition to keep the behavior scoped to actual locations.
🧹 Nitpick comments (1)
backend/internal/sys/validate/notifier_url.go (1)
62-73: 🔒 Security & Privacy | 🔵 TrivialVerify the proxy’s default SSRF policy for user-configured integrations.
This transport enforces the supplied
NotifierConf; the provided config defaults leave localhost/private blocking off. For user-controlled integration URLs, either document the required hardening flags or use a stricter proxy-specific policy if LAN targets are not intentionally allowed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/sys/validate/notifier_url.go` around lines 62 - 73, The NewOutboundHTTPTransport path is using the raw NotifierConf defaults, which leave localhost/private-network blocking disabled for user-configured integrations. Update the notifier URL handling to either apply a stricter proxy-specific outbound policy by default for outboundDialContext/NewOutboundHTTPTransport, or clearly require and document the hardening flags that callers must enable when using user-controlled URLs. Ensure the final policy in notifier_url.go is explicit about whether LAN/private targets are allowed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/internal/sys/validate/notifier_url.go`:
- Around line 54-57: The DNS validation in validateNotifierURL currently uses
context.Background(), which can outlive request timeouts; update the call in
validateNotifierURL to use a bounded context instead. Prefer threading the
caller’s context through the validation path and into resolveOutboundHost, or
otherwise create a deadline-limited context before resolving the host so
outbound DNS lookups respect proxy/request limits.
- Around line 93-106: The network policy check in `validateNotifierURL` is
running before the dial-network filter, so unreachable IP families can fail
validation even when a valid address exists. Update the `validateOutboundIPs`
flow to only consider IPs that match the requested network (`tcp4`/`tcp6`)
before applying block/allow policy, using the existing `ipMatchesNetwork` logic
in `validate/notifier_url.go`. Keep the `resolveOutboundHost`,
`validateOutboundIPs`, and `dialer.DialContext` path intact, but ensure
validation is scoped to the filtered IP list instead of all resolved addresses.
In `@frontend/components/Item/View/table/data-table-expanded-row.vue`:
- Line 15: The expanded row and columns logic is using props.item.parent as if
it were always a location, which can produce invalid /location links for
non-location parents. Update the computed location in
data-table-expanded-row.vue and the related parent-link handling in columns.ts
to only treat parent as a location when parent?.entityType?.isLocation is true,
otherwise leave it unset so the link is not rendered.
In `@frontend/pages/reports/label-generator.vue`:
- Around line 228-241: The location mapping in getItem is too permissive because
item?.parent?.name can populate the label with any parent name instead of only
locations. Update getItem in label-generator.vue to use the same parent entity
type guard as saveAsTemplate, checking item.parent?.entityType?.isLocation
before assigning location, and fall back to labelBlankLine otherwise. Keep the
existing assetID/name behavior unchanged.
---
Outside diff comments:
In `@frontend/components/Item/View/table/columns.ts`:
- Around line 156-163: The link rendering in the table cell currently assumes
item.parent is always a location, which can send users to the wrong page. Update
the cell renderer in columns.ts to only render the /location/${loc.id} anchor
when entityType?.isLocation is true and item.parent exists; otherwise fall back
to the muted placeholder. Use the existing row.original EntitySummary,
item.parent, and entityType check in this column definition to keep the behavior
scoped to actual locations.
---
Nitpick comments:
In `@backend/internal/sys/validate/notifier_url.go`:
- Around line 62-73: The NewOutboundHTTPTransport path is using the raw
NotifierConf defaults, which leave localhost/private-network blocking disabled
for user-configured integrations. Update the notifier URL handling to either
apply a stricter proxy-specific outbound policy by default for
outboundDialContext/NewOutboundHTTPTransport, or clearly require and document
the hardening flags that callers must enable when using user-controlled URLs.
Ensure the final policy in notifier_url.go is explicit about whether LAN/private
targets are allowed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3b3ee73b-1cab-4468-9fe0-a9813dd6cb9d
📒 Files selected for processing (28)
backend/app/api/handlers/v1/controller.gobackend/app/api/handlers/v1/v1_ctrl_integration_proxy.gobackend/internal/core/services/service_items_attachments_external_test.gobackend/internal/sys/config/conf.gobackend/internal/sys/validate/notifier_url.gobackend/internal/sys/validate/notifier_url_test.gofrontend/components/Entity/CreateModal.vuefrontend/components/Item/AttachmentsList.vuefrontend/components/Item/View/ItemChangeDetails.vuefrontend/components/Item/View/table/columns.tsfrontend/components/Item/View/table/data-table-dropdown.vuefrontend/components/Item/View/table/data-table-expanded-row.vuefrontend/components/Scanner/AROverlayCard.vuefrontend/components/Template/Card.vuefrontend/composables/use-barcode-detector.tsfrontend/lib/api/__test__/factories/index.tsfrontend/lib/api/__test__/user/items.test.tsfrontend/lib/api/__test__/user/tags.test.tsfrontend/lib/api/__test__/user/templates.test.tsfrontend/lib/api/classes/items.tsfrontend/lib/integration-adapters.tsfrontend/pages/collection/index/tools.vuefrontend/pages/item/[id]/index.vuefrontend/pages/item/[id]/index/edit.vuefrontend/pages/profile.vuefrontend/pages/reports/label-generator.vuefrontend/pages/template/[id].vuefrontend/stores/integration-cache.ts
✅ Files skipped from review due to trivial changes (2)
- backend/internal/sys/config/conf.go
- frontend/lib/api/test/user/tags.test.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- backend/app/api/handlers/v1/controller.go
- frontend/components/Template/Card.vue
- frontend/pages/template/[id].vue
- frontend/pages/item/[id]/index/edit.vue
- frontend/pages/profile.vue
- frontend/stores/integration-cache.ts
- backend/internal/core/services/service_items_attachments_external_test.go
- backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go
- frontend/lib/integration-adapters.ts
- backend/internal/sys/validate/notifier_url_test.go
- frontend/components/Item/AttachmentsList.vue
Contributor
Author
|
@tankerkiller125 With the latest updates, Homebox's shared outbound URL validation and transport are now used for the integration proxy as requested, replacing the feature-specific SSRF handling. Additionally, CodeRabbit-requested cleanup has been performed, including bounded DNS validation, network-family filtering before policy checks, and parent-as-location guards. Please let me know what other fixes/ architectural changes need to be done. |
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/pages/item/[id]/index.vue (1)
601-608: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve the source field type when saving templates.
These lines now copy the variant values, but Line 604 still forces every saved field to
type: "text". A boolean/number/time custom field saved as a template will therefore round-trip with the wrong schema.Suggested fix
fields: item.value.fields.map(field => ({ id: NIL_UUID, name: field.name, - type: "text", + type: field.type, booleanValue: field.booleanValue, numberValue: field.numberValue, textValue: field.textValue || "", timeValue: DEFAULT_TEMPLATE_FIELD_TIME_VALUE, })),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/pages/item/`[id]/index.vue around lines 601 - 608, The template-save mapping is overriding every field to text instead of preserving each source field’s schema. In the item template creation logic where item.value.fields is mapped, set the saved field type from the original field’s type rather than hardcoding text, while still copying the matching variant value properties so boolean, number, and time fields round-trip correctly.frontend/components/Item/View/table/data-table-dropdown.vue (1)
63-72: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winLocation export is now blank for every row.
With
EntitySummary,item.original["location"]no longer exists, so the CSV path exports an empty field even though the table renders location fromparent. The same raw-key lookup indownloadJson()below has the same regression. Special-case derived columns likelocationwhen building export values.Suggested fix
+ const getExportValue = (item: EntitySummary, col: string) => { + if (col === "location") { + return item.parent?.entityType?.isLocation ? item.parent.name : ""; + } + + return item[col as keyof EntitySummary] ?? ""; + }; + const downloadCsv = (items: Row<EntitySummary>[], columns: Column<EntitySummary>[]) => { @@ const rows = items.map(item => - enabledColumns.map(col => formatValueAsCsvField(item.original[col as keyof EntitySummary])).join(",") + enabledColumns.map(col => formatValueAsCsvField(getExportValue(item.original, col))).join(",") );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/components/Item/View/table/data-table-dropdown.vue` around lines 63 - 72, The export logic in downloadCsv and downloadJson is using raw EntitySummary keys, so derived table columns like location end up blank because item.original["location"] does not exist. Update the row-building logic in these helpers to special-case location and pull the displayed value from parent (the same source the table uses), while keeping the existing key-based lookup for normal fields. Make sure both CSV and JSON export paths use the same derived-value handling so the exported data matches what the table renders.
♻️ Duplicate comments (2)
backend/internal/data/repo/repo_entity_types.go (1)
90-111: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake
ensureDefaultEntityTypeconcurrency-safe.This is still a check-then-create race. A foreground
EnsureDefaultscall and the new background retry path inbackend/internal/core/services/service_user.gocan both miss the same row, then either insert duplicates or surface a constraint error. This helper needs an atomic upsert / constraint-retry path here, plus a unique(group_id, is_location)guarantee if that is not already enforced.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/data/repo/repo_entity_types.go` around lines 90 - 111, `ensureDefaultEntityType` still does a check-then-create flow, so concurrent callers can both miss the same `EntityType` and race on insert. Update the `entityTypes.Query().First` / `entityTypes.Create` path to use an atomic upsert or a retry-on-unique-constraint pattern inside `ensureDefaultEntityType`, and make sure the `EntityType` model enforces a unique `(group_id, is_location)` constraint if it does not already. Keep the return contract the same, but ensure the create path is safe when both `EnsureDefaults` and the retry logic in `service_user.go` run at once.backend/internal/core/services/service_user.go (1)
219-226: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winThe retryable bootstrap still fails the request later in the same flow.
Line 220 and Line 598 downgrade
EnsureDefaultsfailures to log+retry, but Line 257 and Line 635 still callensureDefaultEntityTypes(...)and return the error after the user/group has already been persisted. That reintroduces the partial-success contract break this change is trying to remove. Drop the later hard-fail path or make it follow the same best-effort retry behavior.Also applies to: 597-606
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/core/services/service_user.go` around lines 219 - 226, The `EnsureDefaults` bootstrap path is being treated as retryable in `service_user.go`, but the later `ensureDefaultEntityTypes(...)` call still returns a hard error after the user/group has already been persisted. Update the `register_user` flow so the later `ensureDefaultEntityTypes` branch follows the same best-effort behavior as the earlier `svc.repos.EntityTypes.EnsureDefaults` handling: log, record the span error, schedule `retryEntityTypeDefaults`, and do not propagate the failure as a request error. Keep the behavior consistent across both call sites that currently enforce the default entity type setup.
🧹 Nitpick comments (2)
backend/internal/data/repo/repo_item_attachments_test.go (1)
229-235: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a direct repo test for the unsupported-MIME branch.
The new
!isExternalLink(mimeType)guard inCreateExternalLinkis only covered indirectly today. A dedicated repo test would lock in that contract for any future caller that bypassesMimeTypeForSourceType.🧪 Proposed test
+func TestAttachmentRepo_CreateExternalLink_InvalidMimeType(t *testing.T) { + ctx := context.Background() + entity := useEntities(t, 1)[0] + + _, err := tRepos.Attachments.CreateExternalLink( + ctx, + entity.ID, + "42", + "Bad MIME", + "application/pdf", + attachment.TypeAttachment, + ) + assert.Error(t, err) +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/data/repo/repo_item_attachments_test.go` around lines 229 - 235, Add a direct repository test for the unsupported-MIME guard in CreateExternalLink, since the new !isExternalLink(mimeType) path is only exercised indirectly today. Extend the repo test coverage in TestAttachmentRepo_CreateExternalLink_InvalidEntityID or add a sibling test that calls tRepos.Attachments.CreateExternalLink with a valid entity ID but a MIME type that is not an external link, and assert it returns an error. Use CreateExternalLink and the existing attachment test helpers/cases to keep the test focused on this branch.frontend/composables/use-preferences.ts (1)
2-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollapse the duplicate
preferences-utilsimport/re-export.This import plus
export ... from "./preferences-utils"pattern is what the frontend pipeline is warning about. Re-export the local bindings instead so this file stops generating duplicate-import noise.Minimal cleanup
import { type LocationViewPreferences, type PreferenceSyncConfig, DEFAULT_PREFERENCES, buildSyncedSettings, mergeSyncedSettings, } from "./preferences-utils"; -export type { ViewType, DuplicateSettings, LocationViewPreferences, PreferenceSyncConfig } from "./preferences-utils"; -export { DEFAULT_PREFERENCES, buildSyncedSettings, mergeSyncedSettings } from "./preferences-utils"; +export type { ViewType, DuplicateSettings, LocationViewPreferences, PreferenceSyncConfig }; +export { DEFAULT_PREFERENCES, buildSyncedSettings, mergeSyncedSettings };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/composables/use-preferences.ts` around lines 2 - 11, The use-preferences module is importing from preferences-utils and then re-exporting from the same module, which triggers duplicate-import noise in the frontend pipeline. Update the use-preferences file to re-export the already imported bindings directly, using the local symbols from the import statement instead of a separate export-from clause; keep the existing symbols such as DEFAULT_PREFERENCES, buildSyncedSettings, mergeSyncedSettings, LocationViewPreferences, and PreferenceSyncConfig wired through the same module.Source: Pipeline failures
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go`:
- Around line 65-124: The proxy handler in HandleUserSelfSettingsUpdate only
validates the URL-safe format of name, which still allows arbitrary integration
keys to be used for authenticated outbound requests. Add an explicit allowlist
check for supported integration names before reading {name}_url and
{name}_token, and return a bad request error for anything unsupported. Keep the
existing name validation, but gate the proxy path earlier in the handler that
uses validIntegrationName, settings, and
validate.ValidateOutboundHTTPURLWithContext.
In `@backend/internal/core/services/service_items_attachments.go`:
- Around line 21-36: The trace redaction in redactExternalIdentifierForTrace
still exposes the raw host/port for "link" sources, so update it to redact or
hash the hostname as well before returning the value used for
integration.external_id spans. Keep the existing path hashing logic, but replace
any direct use of u.Host in the formatted trace identifier with a sanitized host
representation so no user-supplied hostname leaks through. Also make sure the
call site that records integration.external_id uses the updated redaction
output.
In `@frontend/components/Entity/CreateModal.vue`:
- Around line 623-634: The template creation path is still using only
form.location?.id for parentId, which ignores the selected parent item and can
place templated sub-items at the wrong level. Update the parentId resolution in
CreateModal.vue so the template branch uses the same fallback logic as the
non-template branch (prefer form.parentId, then location as fallback), and apply
the same fix in the template request construction around the template creation
flow.
In `@frontend/components/Item/AttachmentsList.vue`:
- Around line 72-85: The AttachmentsList.vue template still contains hardcoded
English literals, so move the remaining `pages` text and the `aria-label` on the
`MdiAlertCircleOutline` inside the item actions behind existing i18n usage.
Update the relevant template expressions in `AttachmentsList.vue` to pull
translated strings through the same translation mechanism used elsewhere in the
component, keeping the `paperlessDoc(attachment)` and
`isUnreachable(attachment)` logic unchanged.
- Around line 113-140: The external-link rendering branch in AttachmentsList.vue
is binding attachment.path directly into anchor hrefs without validating the
scheme. Update the isLink(attachment) path in AttachmentsList to reuse the
existing HTTP(S) URL check used elsewhere in the item attachment flow, and only
render the two anchors when the URL is safe. If attachment.path is not a valid
HTTP(S) URL, fall back to plain text rendering instead of a clickable link.
In `@frontend/components/Item/View/ItemChangeDetails.vue`:
- Around line 61-65: Reset newLocation before computing the default selection in
ItemChangeDetails.vue so reopening the dialog cannot reuse a stale parent from a
previous run. In the dialog setup/watch logic where params.changeLocation and
params.items are evaluated, clear newLocation first and only assign firstParent
when all selected items share the same location parent; otherwise leave it unset
so save() cannot submit an old parentId.
In `@frontend/composables/preferences-utils.ts`:
- Around line 65-100: The synced preference key list is incorrectly derived from
DEFAULT_PREFERENCES, so optional LocationViewPreferences fields like
tableHeaders and collectionId are skipped during serialization and merge. Update
preferences-utils.ts to source the syncable keys from the full
LocationViewPreferences shape or an explicit shared key list instead of
DEFAULT_PREFERENCES, and make forEachSyncedPreference, buildSyncedSettings, and
mergeSyncedSettings operate over that complete set so enabled optional
preferences are preserved.
In `@frontend/composables/use-preferences.ts`:
- Around line 88-90: Abort the save flow in use-preferences when
api.user.getSettings() fails or returns no item, instead of building merged from
an empty object. Update the logic around buildSyncedSettings and
api.user.setSettings so the write only proceeds after a successful read with
current.item present; otherwise return early and preserve existing settings,
including unrelated integrations and tokens.
In `@frontend/lib/integration-adapters.ts`:
- Around line 50-65: In integration-adapters.ts, the helper that computes the
extracted ID currently falls back to pattern-only matching even when a
configured baseUrl is present but invalid, which can incorrectly accept links;
update the logic around the URL parsing block and the path extraction in this
function so that any non-empty baseUrl that fails to parse returns null
immediately, while preserving the existing heuristic pattern-only fallback only
when baseUrl is undefined. Use the existing URL matching flow in this helper
(including target.origin, basePath, and the final pathAfterBase.match call) to
keep the valid configured-base behavior unchanged.
In `@frontend/pages/reports/label-generator.vue`:
- Around line 268-271: The top-level item path in label generation is being
altered by passing null into getItem, which clears the asset ID and name for
root items. In label-generator.vue, update the branch around getItem so that
items without item.parent are still passed through as the original item, letting
getItem() handle the missing-location case itself; keep the behavior for
parented items unchanged.
In `@frontend/stores/integration-cache.ts`:
- Around line 110-128: The enrichment cache key is too broad because
getEnrichedData, setEnrichedData, and invalidateEnrichedData only namespace by
serviceName and id, so persisted localStorage entries can leak across users or
different Paperless URLs. Update the cache keying in integration-cache.ts to
include the authenticated user id and a normalized service base URL, or
alternatively clear all enrichment entries when setServiceUrl() changes to a new
backend. Make sure the same namespacing/clear behavior is applied consistently
in the read, write, and invalidate paths so stale metadata cannot be reused
across accounts or service instances.
---
Outside diff comments:
In `@frontend/components/Item/View/table/data-table-dropdown.vue`:
- Around line 63-72: The export logic in downloadCsv and downloadJson is using
raw EntitySummary keys, so derived table columns like location end up blank
because item.original["location"] does not exist. Update the row-building logic
in these helpers to special-case location and pull the displayed value from
parent (the same source the table uses), while keeping the existing key-based
lookup for normal fields. Make sure both CSV and JSON export paths use the same
derived-value handling so the exported data matches what the table renders.
In `@frontend/pages/item/`[id]/index.vue:
- Around line 601-608: The template-save mapping is overriding every field to
text instead of preserving each source field’s schema. In the item template
creation logic where item.value.fields is mapped, set the saved field type from
the original field’s type rather than hardcoding text, while still copying the
matching variant value properties so boolean, number, and time fields round-trip
correctly.
---
Duplicate comments:
In `@backend/internal/core/services/service_user.go`:
- Around line 219-226: The `EnsureDefaults` bootstrap path is being treated as
retryable in `service_user.go`, but the later `ensureDefaultEntityTypes(...)`
call still returns a hard error after the user/group has already been persisted.
Update the `register_user` flow so the later `ensureDefaultEntityTypes` branch
follows the same best-effort behavior as the earlier
`svc.repos.EntityTypes.EnsureDefaults` handling: log, record the span error,
schedule `retryEntityTypeDefaults`, and do not propagate the failure as a
request error. Keep the behavior consistent across both call sites that
currently enforce the default entity type setup.
In `@backend/internal/data/repo/repo_entity_types.go`:
- Around line 90-111: `ensureDefaultEntityType` still does a check-then-create
flow, so concurrent callers can both miss the same `EntityType` and race on
insert. Update the `entityTypes.Query().First` / `entityTypes.Create` path to
use an atomic upsert or a retry-on-unique-constraint pattern inside
`ensureDefaultEntityType`, and make sure the `EntityType` model enforces a
unique `(group_id, is_location)` constraint if it does not already. Keep the
return contract the same, but ensure the create path is safe when both
`EnsureDefaults` and the retry logic in `service_user.go` run at once.
---
Nitpick comments:
In `@backend/internal/data/repo/repo_item_attachments_test.go`:
- Around line 229-235: Add a direct repository test for the unsupported-MIME
guard in CreateExternalLink, since the new !isExternalLink(mimeType) path is
only exercised indirectly today. Extend the repo test coverage in
TestAttachmentRepo_CreateExternalLink_InvalidEntityID or add a sibling test that
calls tRepos.Attachments.CreateExternalLink with a valid entity ID but a MIME
type that is not an external link, and assert it returns an error. Use
CreateExternalLink and the existing attachment test helpers/cases to keep the
test focused on this branch.
In `@frontend/composables/use-preferences.ts`:
- Around line 2-11: The use-preferences module is importing from
preferences-utils and then re-exporting from the same module, which triggers
duplicate-import noise in the frontend pipeline. Update the use-preferences file
to re-export the already imported bindings directly, using the local symbols
from the import statement instead of a separate export-from clause; keep the
existing symbols such as DEFAULT_PREFERENCES, buildSyncedSettings,
mergeSyncedSettings, LocationViewPreferences, and PreferenceSyncConfig wired
through the same module.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ec5c70ad-e4b3-49c5-b0c8-71fef7b00e49
⛔ Files ignored due to path filters (1)
backend/go.sumis excluded by!**/*.sum
📒 Files selected for processing (49)
backend/app/api/handlers/v1/controller.gobackend/app/api/handlers/v1/v1_ctrl_integration_proxy.gobackend/app/api/handlers/v1/v1_ctrl_integration_proxy_test.gobackend/app/api/routes.gobackend/internal/core/services/main_test.gobackend/internal/core/services/service_group.gobackend/internal/core/services/service_items_attachments.gobackend/internal/core/services/service_items_attachments_external_test.gobackend/internal/core/services/service_user.gobackend/internal/data/repo/main_test.gobackend/internal/data/repo/repo_entity_types.gobackend/internal/data/repo/repo_entity_types_test.gobackend/internal/data/repo/repo_group.gobackend/internal/data/repo/repo_item_attachments.gobackend/internal/data/repo/repo_item_attachments_test.gobackend/internal/sys/config/conf.gobackend/internal/sys/validate/notifier_url.gobackend/internal/sys/validate/notifier_url_test.godocs/src/content/docs/en/user-guide/notifiers.mdxfrontend/components/Entity/CreateModal.vuefrontend/components/Item/AttachmentsList.vuefrontend/components/Item/View/ItemChangeDetails.vuefrontend/components/Item/View/table/columns.tsfrontend/components/Item/View/table/data-table-dropdown.vuefrontend/components/Item/View/table/data-table-expanded-row.vuefrontend/components/Scanner/AROverlayCard.vuefrontend/components/Template/Card.vuefrontend/components/Template/CreateModal.vuefrontend/composables/preferences-utils.tsfrontend/composables/use-barcode-detector.tsfrontend/composables/use-preferences.test.tsfrontend/composables/use-preferences.tsfrontend/lib/api/__test__/factories/index.tsfrontend/lib/api/__test__/user/items.test.tsfrontend/lib/api/__test__/user/stats.test.tsfrontend/lib/api/__test__/user/tags.test.tsfrontend/lib/api/__test__/user/templates.test.tsfrontend/lib/api/classes/items.tsfrontend/lib/integration-adapters.test.tsfrontend/lib/integration-adapters.tsfrontend/lib/template-fields.tsfrontend/locales/en.jsonfrontend/pages/collection/index/tools.vuefrontend/pages/item/[id]/index.vuefrontend/pages/item/[id]/index/edit.vuefrontend/pages/profile.vuefrontend/pages/reports/label-generator.vuefrontend/pages/template/[id].vuefrontend/stores/integration-cache.ts
Comment on lines
+65
to
+124
| name := chi.URLParam(r, "name") | ||
| if !validIntegrationName.MatchString(name) { | ||
| return validate.NewRequestError(fmt.Errorf("invalid integration name"), http.StatusBadRequest) | ||
| } | ||
|
|
||
| rawPath := r.URL.Query().Get("path") | ||
| if rawPath == "" { | ||
| return validate.NewRequestError(fmt.Errorf("path query parameter is required"), http.StatusBadRequest) | ||
| } | ||
| if !strings.HasPrefix(rawPath, "/") || strings.Contains(rawPath, "://") { | ||
| return validate.NewRequestError(fmt.Errorf("path must be a relative path starting with /"), http.StatusBadRequest) | ||
| } | ||
|
|
||
| // Normalise to prevent directory traversal while preserving trailing slash | ||
| // (many REST APIs treat /foo/1/ and /foo/1 differently). | ||
| cleanPath := path.Clean(rawPath) | ||
| if !strings.HasPrefix(cleanPath, "/") { | ||
| return validate.NewRequestError(fmt.Errorf("invalid path after normalisation"), http.StatusBadRequest) | ||
| } | ||
| if strings.HasSuffix(rawPath, "/") && !strings.HasSuffix(cleanPath, "/") { | ||
| cleanPath += "/" | ||
| } | ||
|
|
||
| span.SetAttributes( | ||
| attribute.String("integration.name", name), | ||
| attribute.String("integration.path", cleanPath), | ||
| ) | ||
|
|
||
| ctx := services.NewContext(spanCtx) | ||
| settings, svcErr := ctrl.svc.User.GetSettings(ctx.Context, services.UseUserCtx(ctx.Context).ID) | ||
| if svcErr != nil { | ||
| return validate.NewRequestError(svcErr, http.StatusInternalServerError) | ||
| } | ||
|
|
||
| baseURL, _ := settings[name+"_url"].(string) | ||
| if baseURL == "" { | ||
| return validate.NewRequestError( | ||
| fmt.Errorf("%s_url not configured – add it in Settings", name), | ||
| http.StatusBadRequest, | ||
| ) | ||
| } | ||
|
|
||
| upstream := strings.TrimRight(baseURL, "/") + cleanPath | ||
| if err := validate.ValidateOutboundHTTPURLWithContext(r.Context(), upstream, &ctrl.config.Notifier); err != nil { | ||
| return validate.NewRequestError(err, http.StatusBadRequest) | ||
| } | ||
|
|
||
| token, _ := settings[name+"_token"].(string) | ||
| if token == "" { | ||
| return validate.NewRequestError( | ||
| fmt.Errorf("%s_token not configured – add it in Settings", name), | ||
| http.StatusBadRequest, | ||
| ) | ||
| } | ||
|
|
||
| req, err := http.NewRequestWithContext(r.Context(), http.MethodGet, upstream, nil) | ||
| if err != nil { | ||
| return validate.NewRequestError(err, http.StatusBadRequest) | ||
| } | ||
| req.Header.Set("Authorization", "Token "+token) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline backend/app/api/handlers/v1 --match HandleUserSelfSettingsUpdate --view expanded
rg -n -C3 'HandleUserSelfSettingsUpdate|GetSettings|SetSettings|UpdateSettings|validIntegrationName|"_url"|"_token"' \
backend/app/api/handlers/v1 backend/internalRepository: sysadminsmedia/homebox
Length of output: 13671
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the user settings update path and any server-side validation/allowlist for settings keys.
sed -n '205,260p' backend/app/api/handlers/v1/v1_ctrl_user.go
printf '\n---\n'
sed -n '1,220p' backend/internal/core/services/service_user.go | sed -n '790,840p'
printf '\n---\n'
sed -n '340,390p' backend/internal/data/repo/repo_users.go
printf '\n---\n'
rg -n -C2 'paperless|integrat|_url"|_token"|settings.*allow|allowlist|whitelist|validation.*settings|map\[string\]interface' backend/app backend/internalRepository: sysadminsmedia/homebox
Length of output: 40809
🏁 Script executed:
#!/bin/bash
python3 - <<'PY'
from pathlib import Path
p = Path('backend/app/api/handlers/v1/v1_ctrl_user.go')
print(p.exists())
PYRepository: sysadminsmedia/homebox
Length of output: 165
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '205,260p' backend/app/api/handlers/v1/v1_ctrl_user.go
printf '\n---\n'
sed -n '760,815p' backend/internal/core/services/service_user.go
printf '\n---\n'
sed -n '363,380p' backend/internal/data/repo/repo_users.go
printf '\n---\n'
rg -n -C2 'validIntegrationName|name\\+\"_url\"|name\\+\"_token\"|SetSettings\\(|GetSettings\\(' backend/app/api/handlers/v1 backend/internalRepository: sysadminsmedia/homebox
Length of output: 5797
🏁 Script executed:
#!/bin/bash
python3 - <<'PY'
from pathlib import Path
p = Path('backend/app/api/handlers/v1/v1_ctrl_user.go')
print(p.read_text().splitlines()[204:225])
PYRepository: sysadminsmedia/homebox
Length of output: 1350
Gate name to supported integrations. HandleUserSelfSettingsUpdate persists arbitrary settings keys, so regex-only validation still allows any lower-case {name} to become an authenticated outbound proxy via {name}_url / {name}_token.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/api/handlers/v1/v1_ctrl_integration_proxy.go` around lines 65 -
124, The proxy handler in HandleUserSelfSettingsUpdate only validates the
URL-safe format of name, which still allows arbitrary integration keys to be
used for authenticated outbound requests. Add an explicit allowlist check for
supported integration names before reading {name}_url and {name}_token, and
return a bad request error for anything unsupported. Keep the existing name
validation, but gate the proxy path earlier in the handler that uses
validIntegrationName, settings, and validate.ValidateOutboundHTTPURLWithContext.
Comment on lines
+65
to
+100
| const preferenceKeys = Object.keys(DEFAULT_PREFERENCES) as (keyof LocationViewPreferences)[]; | ||
|
|
||
| export function forEachSyncedPreference( | ||
| syncConfig: PreferenceSyncConfig, | ||
| callback: (key: keyof LocationViewPreferences) => void | ||
| ) { | ||
| for (const key of preferenceKeys) { | ||
| if (syncConfig[key] !== false) { | ||
| callback(key); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| export function buildSyncedSettings( | ||
| preferences: LocationViewPreferences, | ||
| syncConfig: PreferenceSyncConfig | ||
| ): Record<string, unknown> { | ||
| const payload: Record<string, unknown> = {}; | ||
| forEachSyncedPreference(syncConfig, key => { | ||
| payload[key] = preferences[key]; | ||
| }); | ||
| return payload; | ||
| } | ||
|
|
||
| export function mergeSyncedSettings( | ||
| settings: Record<string, unknown>, | ||
| preferences: LocationViewPreferences, | ||
| syncConfig: PreferenceSyncConfig | ||
| ): LocationViewPreferences { | ||
| const nextPreferences = { ...preferences }; | ||
| forEachSyncedPreference(syncConfig, key => { | ||
| if (key in settings) { | ||
| nextPreferences[key] = settings[key] as never; | ||
| } | ||
| }); | ||
| return nextPreferences; |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Don't derive syncable keys from the default object.
preferenceKeys only includes keys present in DEFAULT_PREFERENCES, so optional LocationViewPreferences members like tableHeaders and collectionId can never be serialized or merged even when they exist and syncConfig enables them. That makes the exported sync contract lie and silently drops valid settings.
Suggested direction
-const preferenceKeys = Object.keys(DEFAULT_PREFERENCES) as (keyof LocationViewPreferences)[];
+const preferenceKeys: (keyof LocationViewPreferences)[] = [
+ "showDetails",
+ "showEmpty",
+ "editorAdvancedView",
+ "itemDisplayView",
+ "theme",
+ "itemsPerTablePage",
+ "tableHeaders",
+ "displayLegacyHeader",
+ "legacyImageFit",
+ "language",
+ "overrideFormatLocale",
+ "collectionId",
+ "duplicateSettings",
+ "shownMultiTabWarning",
+ "quickActions",
+];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const preferenceKeys = Object.keys(DEFAULT_PREFERENCES) as (keyof LocationViewPreferences)[]; | |
| export function forEachSyncedPreference( | |
| syncConfig: PreferenceSyncConfig, | |
| callback: (key: keyof LocationViewPreferences) => void | |
| ) { | |
| for (const key of preferenceKeys) { | |
| if (syncConfig[key] !== false) { | |
| callback(key); | |
| } | |
| } | |
| } | |
| export function buildSyncedSettings( | |
| preferences: LocationViewPreferences, | |
| syncConfig: PreferenceSyncConfig | |
| ): Record<string, unknown> { | |
| const payload: Record<string, unknown> = {}; | |
| forEachSyncedPreference(syncConfig, key => { | |
| payload[key] = preferences[key]; | |
| }); | |
| return payload; | |
| } | |
| export function mergeSyncedSettings( | |
| settings: Record<string, unknown>, | |
| preferences: LocationViewPreferences, | |
| syncConfig: PreferenceSyncConfig | |
| ): LocationViewPreferences { | |
| const nextPreferences = { ...preferences }; | |
| forEachSyncedPreference(syncConfig, key => { | |
| if (key in settings) { | |
| nextPreferences[key] = settings[key] as never; | |
| } | |
| }); | |
| return nextPreferences; | |
| const preferenceKeys: (keyof LocationViewPreferences)[] = [ | |
| "showDetails", | |
| "showEmpty", | |
| "editorAdvancedView", | |
| "itemDisplayView", | |
| "theme", | |
| "itemsPerTablePage", | |
| "tableHeaders", | |
| "displayLegacyHeader", | |
| "legacyImageFit", | |
| "language", | |
| "overrideFormatLocale", | |
| "collectionId", | |
| "duplicateSettings", | |
| "shownMultiTabWarning", | |
| "quickActions", | |
| ]; | |
| export function forEachSyncedPreference( | |
| syncConfig: PreferenceSyncConfig, | |
| callback: (key: keyof LocationViewPreferences) => void | |
| ) { | |
| for (const key of preferenceKeys) { | |
| if (syncConfig[key] !== false) { | |
| callback(key); | |
| } | |
| } | |
| } | |
| export function buildSyncedSettings( | |
| preferences: LocationViewPreferences, | |
| syncConfig: PreferenceSyncConfig | |
| ): Record<string, unknown> { | |
| const payload: Record<string, unknown> = {}; | |
| forEachSyncedPreference(syncConfig, key => { | |
| payload[key] = preferences[key]; | |
| }); | |
| return payload; | |
| } | |
| export function mergeSyncedSettings( | |
| settings: Record<string, unknown>, | |
| preferences: LocationViewPreferences, | |
| syncConfig: PreferenceSyncConfig | |
| ): LocationViewPreferences { | |
| const nextPreferences = { ...preferences }; | |
| forEachSyncedPreference(syncConfig, key => { | |
| if (key in settings) { | |
| nextPreferences[key] = settings[key] as never; | |
| } | |
| }); | |
| return nextPreferences; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/composables/preferences-utils.ts` around lines 65 - 100, The synced
preference key list is incorrectly derived from DEFAULT_PREFERENCES, so optional
LocationViewPreferences fields like tableHeaders and collectionId are skipped
during serialization and merge. Update preferences-utils.ts to source the
syncable keys from the full LocationViewPreferences shape or an explicit shared
key list instead of DEFAULT_PREFERENCES, and make forEachSyncedPreference,
buildSyncedSettings, and mergeSyncedSettings operate over that complete set so
enabled optional preferences are preserved.
Comment on lines
+50
to
+65
| if (baseUrl?.trim()) { | ||
| try { | ||
| const base = new URL(baseUrl.trim()); | ||
| if (base.origin !== target.origin) return null; | ||
| basePath = base.pathname.replace(/\/$/, ""); | ||
| if (basePath && !target.pathname.startsWith(basePath + "/") && target.pathname !== basePath) { | ||
| return null; | ||
| } | ||
| } catch (e) { | ||
| // Invalid configured base URL – fall through to pattern-only match. | ||
| console.warn("integration-adapters: invalid baseUrl, falling back to pattern-only match:", baseUrl, e); | ||
| } | ||
| } | ||
|
|
||
| const pathAfterBase = target.pathname.slice(basePath.length || 0); | ||
| return pathAfterBase.match(pattern)?.[1] ?? null; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not treat an invalid configured base URL as a match.
When baseUrl is supplied but unparseable, this helper still falls back to pattern-only extraction. That makes extractId() promote any /documents/{id} link when the saved Paperless URL is malformed, even though detectServiceFromUrl() now intentionally returns null unless the configured base URL parses and matches. Return null once a configured URL is present but invalid, and keep the heuristic fallback only for the baseUrl === undefined case.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/lib/integration-adapters.ts` around lines 50 - 65, In
integration-adapters.ts, the helper that computes the extracted ID currently
falls back to pattern-only matching even when a configured baseUrl is present
but invalid, which can incorrectly accept links; update the logic around the URL
parsing block and the path extraction in this function so that any non-empty
baseUrl that fails to parse returns null immediately, while preserving the
existing heuristic pattern-only fallback only when baseUrl is undefined. Use the
existing URL matching flow in this helper (including target.origin, basePath,
and the final pathAfterBase.match call) to keep the valid configured-base
behavior unchanged.
Comment on lines
+110
to
+128
| function getEnrichedData(serviceName: string, id: string): unknown { | ||
| const key = `${serviceName}:${id}`; | ||
| if (key in enrichedData) return enrichedData[key]; | ||
| const cached = lsRead(key); | ||
| if (cached !== null) enrichedData[key] = cached; | ||
| return cached; | ||
| } | ||
|
|
||
| function setEnrichedData(serviceName: string, id: string, data: unknown): void { | ||
| const key = `${serviceName}:${id}`; | ||
| enrichedData[key] = data; | ||
| lsWrite(key, data); | ||
| } | ||
|
|
||
| function invalidateEnrichedData(serviceName: string, id: string): void { | ||
| const key = `${serviceName}:${id}`; | ||
| Reflect.deleteProperty(enrichedData, key); | ||
| lsDelete(key); | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Scope cached enrichment data to the user and the configured service URL.
The cache key is only ${serviceName}:${id}. Because this store persists that data in localStorage, a second Homebox account on the same browser profile—or the same account after repointing paperless_url at a different Paperless instance—can reuse the previous title/tags for the same document id. If the new service is unreachable, that stale metadata survives for the full TTL. Please namespace the cache with at least the authenticated user id and normalized base URL, or clear the service cache whenever setServiceUrl() changes.
Also applies to: 148-150
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/stores/integration-cache.ts` around lines 110 - 128, The enrichment
cache key is too broad because getEnrichedData, setEnrichedData, and
invalidateEnrichedData only namespace by serviceName and id, so persisted
localStorage entries can leak across users or different Paperless URLs. Update
the cache keying in integration-cache.ts to include the authenticated user id
and a normalized service base URL, or alternatively clear all enrichment entries
when setServiceUrl() changes to a new backend. Make sure the same
namespacing/clear behavior is applied consistently in the read, write, and
invalidate paths so stale metadata cannot be reused across accounts or service
instances.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/pages/item/[id]/index/edit.vue (2)
558-571: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDefaulting
entityTypeIdto an empty string risks corrupting the entity's type on save.
entityTypeId: item.value.entityType?.id || ""will submit an empty string toapi.items.updateifentityTypeis ever missing, whereassaveItemtreats this field as required (item.value.entityType!.id). Sending""could either fail validation or, worse, silently clear/break the entity-type association during a location sync — a data-integrity risk distinct from a simple no-op.Consider guarding and aborting (similar to the existing
!item.value.parent?.idcheck just above) instead of silently defaulting:🛡️ Suggested fix
+ if (!item.value.entityType?.id) { + toast.error(t("items.toast.failed_save")); + return; + } const payload: EntityUpdate = { ...item.value, parentId: parent.value?.id || item.value.parent?.id || null, tagIds: item.value.tagIds, assetId: item.value.assetId, - entityTypeId: item.value.entityType?.id || "", + entityTypeId: item.value.entityType.id, syncChildEntityLocations: item.value.syncChildEntityLocations, };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/pages/item/`[id]/index/edit.vue around lines 558 - 571, The syncChildEntityLocations flow is defaulting entityTypeId to an empty string, which can corrupt the item’s type on update. In syncChildEntityLocations, replace the fallback on item.value.entityType?.id with a required guard like the existing parent/location check, and abort with a toast or error if entityType is missing before calling api.items.update. Keep the payload construction in sync with saveItem by ensuring entityTypeId is only sent when item.value.entityType is present.
110-120: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove leftover debug
console.logstatements.Lines 119-120 log
purchasePrice/soldPriceto the console via a redundant??=mutation —purchasePrice/soldPriceare already correctly computed just above (lines 110-117). This looks like a debugging leftover; it's dead logic and needlessly surfaces monetary data in the browser console.🧹 Suggested fix
- console.log((item.value.purchasePrice ??= 0)); - console.log((item.value.soldPrice ??= 0)); -🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/pages/item/`[id]/index/edit.vue around lines 110 - 120, Remove the leftover debug console output in the item edit page: the `console.log` calls in the `index/edit.vue` logic are redundant because `purchasePrice` and `soldPrice` are already computed above. Delete the two `console.log` statements and avoid using the `??=` mutations there, keeping the existing price calculation flow in that block unchanged.
🧹 Nitpick comments (5)
frontend/lib/preferences-utils.ts (2)
65-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winManual key list can drift from
LocationViewPreferences/DEFAULT_PREFERENCES.
preferenceKeysis now hand-maintained instead of derived fromObject.keys(DEFAULT_PREFERENCES). If a new field is later added toLocationViewPreferences/DEFAULT_PREFERENCESbut this array is forgotten, it will silently stop syncing (no compile error since it's just a subset list).♻️ Suggested guard to keep the list in sync
-const preferenceKeys: (keyof LocationViewPreferences)[] = [ - "showDetails", - "showEmpty", - "editorAdvancedView", - "itemDisplayView", - "theme", - "itemsPerTablePage", - "tableHeaders", - "displayLegacyHeader", - "legacyImageFit", - "language", - "overrideFormatLocale", - "collectionId", - "duplicateSettings", - "shownMultiTabWarning", - "quickActions", -]; +const preferenceKeys: (keyof LocationViewPreferences)[] = [ + "showDetails", + "showEmpty", + "editorAdvancedView", + "itemDisplayView", + "theme", + "itemsPerTablePage", + "tableHeaders", + "displayLegacyHeader", + "legacyImageFit", + "language", + "overrideFormatLocale", + "collectionId", + "duplicateSettings", + "shownMultiTabWarning", + "quickActions", +]; + +// Compile-time guard: fails the build if a key exists in DEFAULT_PREFERENCES +// but is missing from preferenceKeys (or vice versa). +type _AssertPreferenceKeysComplete = Exclude<keyof LocationViewPreferences, (typeof preferenceKeys)[number]> extends never + ? true + : false & { error: "preferenceKeys is missing a key from LocationViewPreferences" };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/lib/preferences-utils.ts` around lines 65 - 81, The manually maintained preferenceKeys list can drift from LocationViewPreferences/DEFAULT_PREFERENCES and silently miss new settings. Update the logic in preferences-utils.ts so preferenceKeys is derived from DEFAULT_PREFERENCES (or another single source of truth) instead of a hardcoded subset, and keep the existing sync behavior in the helper that uses preferenceKeys so newly added preference fields are included automatically.
105-119: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSecurity note: allowlist is good, but consider validating value shapes too.
Restricting assignment to keys from the fixed
preferenceKeysarray (rather than iteratingsettings's own keys) is a solid defense against prototype-pollution-style key injection from server JSON — good call. That said, the values themselves (e.g.duplicateSettings,quickActions,tableHeaders) are cast withas neverand copied without shape/type validation. A malformed or unexpected value type from the server could propagate into app state and cause runtime errors in consumers that assume the well-typed shape.Consider adding lightweight runtime validation (e.g.
typeof/shape checks or a schema validator) for structurally complex keys before assignment, as defense-in-depth against unexpected server payloads.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/lib/preferences-utils.ts` around lines 105 - 119, The mergeSyncedSettings function currently allowlists keys via forEachSyncedPreference, but it still copies server-provided values directly into nextPreferences with only a cast, so malformed shapes can flow into app state. Add lightweight runtime validation before assigning each synced value, especially for structurally complex preference keys like duplicateSettings, quickActions, and tableHeaders. Keep the existing key allowlist, but gate assignment on basic typeof/shape checks or a small schema helper so only expected value shapes are merged.backend/app/api/handlers/v1/v1_ctrl_integration_cards.go (1)
100-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate URL-validation helper.
parseHTTPURLre-implements the same scheme/host/no-userinfo checks asparseExternalHTTPURLinv1_ctrl_entities_attachments_external.go. Both enforce the same security invariant (must be http/https, no embedded credentials); consolidating into one shared helper avoids the two drifting apart over time.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/api/handlers/v1/v1_ctrl_integration_cards.go` around lines 100 - 112, The URL validation logic in parseHTTPURL duplicates the same http/https, host, and no-userinfo checks already implemented in parseExternalHTTPURL, so consolidate them into a single shared helper. Update parseHTTPURL to call the existing shared validation path (or extract a common helper used by both parseHTTPURL and parseExternalHTTPURL) so the security invariant stays consistent and avoids future drift.frontend/components/Item/AttachmentsList.vue (2)
43-52: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueSecurity note: tag color/textColor are bound as raw inline styles from an external source.
tag.color/tag.textColororiginate from the Paperless API response and flow straight into:style. While Vue's binding limits this to CSS (no script execution), unsanitized values could still enable minor CSS-based data exfiltration tricks. Consider validating the values look like colors (e.g., a hex/rgb()regex) before binding.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/components/Item/AttachmentsList.vue` around lines 43 - 52, The tag rendering in AttachmentsList.vue binds tag.color and tag.textColor directly into inline styles, so validate/sanitize these values before passing them to :style. Update the cardTags(attachment) usage or add a small helper in the AttachmentsList component to accept only safe color formats (for example, hex or rgb/rgba) and fall back to default styling when values are invalid.
63-90: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider validating
openUrlbefore binding tohref, mirroring thesafeLinkURLguard used below.
integrationCard(attachment)?.openUrlis bound directly into an anchorhrefwithtarget="_blank". It's server-generated today, but this component previously required the same guard forattachment.pathon the exact same security grounds (untrusted scheme → active link). As integrations expand (e.g., Immich per the PR roadmap), a defense-in-depth check here would prevent a future provider bug from becoming a client-side issue.🔒 Suggested guard
- :href="integrationCard(attachment)?.openUrl" + :href="safeOpenURL(integrationCard(attachment))"And reuse (or generalize) the existing URL-validation logic used for
safeLinkURL.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/components/Item/AttachmentsList.vue` around lines 63 - 90, The open-in-new-tab anchor in AttachmentsList.vue binds integrationCard(attachment)?.openUrl directly to href without the same safety check used for attachment.path. Update the rendering around the TooltipTrigger/a link to validate openUrl with the existing safeLinkURL logic (or a shared/generalized helper) before assigning it to href, and only render the active link when the URL passes validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/api/handlers/v1/v1_ctrl_integration_cards_test.go`:
- Around line 14-63: The test table in v1_ctrl_integration_cards_test.go repeats
the same base URL literal multiple times, triggering goconst. Introduce a
package-level test constant for the shared Paperless base URL and update the
affected cases in the integration card matching tests to use it. Keep the
existing symbols like the table-driven test cases in the v1 controller
integration card tests so the new constant is easy to apply across the repeated
entries.
In `@backend/app/api/handlers/v1/v1_ctrl_integration_cards.go`:
- Around line 204-317: The card-building path is doing many sequential upstream
Paperless requests in buildPaperlessCard and
HandleEntityAttachmentIntegrationCards, which can make the endpoint stall on
entities with multiple tagged documents. Refactor buildPaperlessCard to fetch
the optional correspondent, documentType, and tag details concurrently, and add
request-scoped caching for repeated IDs so the same lookups are not repeated
across cards. Also ensure the overall handler uses a shared deadline or timeout
for the full card-building pass so one slow attachment cannot block the entire
response.
- Around line 373-376: The thumbnail response in the integration card handler is
blindly forwarding the upstream Content-Type from
resp.Header.Get("Content-Type"), which can expose this authenticated endpoint to
non-image content from a user-configurable paperless_url. Update the response
handling in the thumbnail path (around the io.Copy(w, resp.Body) flow) to only
pass through an allow-list of safe image MIME types and otherwise fall back to a
safe default or omit the header, and add X-Content-Type-Options: nosniff before
writing the body.
- Around line 87-98: The Paperless integration is still inheriting the shared
outbound policy, which leaves RFC1918 and localhost destinations reachable by
default. Update paperlessConfigFromSettings (and any Paperless URL validation it
relies on) to explicitly reject local-network and localhost targets for
paperless_url unless the relevant HBOX_NOTIFIER_BLOCK_LOCAL_NETS and
HBOX_NOTIFIER_BLOCK_LOCALHOST protections are enabled, so authenticated users
cannot point this integration at internal services.
In `@frontend/components/Item/AttachmentsList.vue`:
- Around line 205-214: `loadIntegrationCards` in `AttachmentsList.vue` can apply
out-of-order results when multiple `watch(attachmentSignature, ...)` updates
overlap. Add a latest-request guard in `loadIntegrationCards` (or around the
`api.items.attachments.integrationCards` call) so only the most recent fetch is
allowed to assign to `cards.value`, and ignore stale responses from earlier
requests.
In `@frontend/pages/profile.vue`:
- Around line 253-256: The settings loader in profile.vue is still using unsafe
string casts for paperless_url and paperless_token, so mirror the hardened
pattern already used in edit.vue by checking the runtime type before assignment.
Update the logic around integrationSettings.paperless_url and
integrationSettings.paperless_token to only accept values when typeof is
"string", and otherwise fall back to an empty string. Use the existing settings
object in this block to keep the fix localized and consistent with the other
loader.
---
Outside diff comments:
In `@frontend/pages/item/`[id]/index/edit.vue:
- Around line 558-571: The syncChildEntityLocations flow is defaulting
entityTypeId to an empty string, which can corrupt the item’s type on update. In
syncChildEntityLocations, replace the fallback on item.value.entityType?.id with
a required guard like the existing parent/location check, and abort with a toast
or error if entityType is missing before calling api.items.update. Keep the
payload construction in sync with saveItem by ensuring entityTypeId is only sent
when item.value.entityType is present.
- Around line 110-120: Remove the leftover debug console output in the item edit
page: the `console.log` calls in the `index/edit.vue` logic are redundant
because `purchasePrice` and `soldPrice` are already computed above. Delete the
two `console.log` statements and avoid using the `??=` mutations there, keeping
the existing price calculation flow in that block unchanged.
---
Nitpick comments:
In `@backend/app/api/handlers/v1/v1_ctrl_integration_cards.go`:
- Around line 100-112: The URL validation logic in parseHTTPURL duplicates the
same http/https, host, and no-userinfo checks already implemented in
parseExternalHTTPURL, so consolidate them into a single shared helper. Update
parseHTTPURL to call the existing shared validation path (or extract a common
helper used by both parseHTTPURL and parseExternalHTTPURL) so the security
invariant stays consistent and avoids future drift.
In `@frontend/components/Item/AttachmentsList.vue`:
- Around line 43-52: The tag rendering in AttachmentsList.vue binds tag.color
and tag.textColor directly into inline styles, so validate/sanitize these values
before passing them to :style. Update the cardTags(attachment) usage or add a
small helper in the AttachmentsList component to accept only safe color formats
(for example, hex or rgb/rgba) and fall back to default styling when values are
invalid.
- Around line 63-90: The open-in-new-tab anchor in AttachmentsList.vue binds
integrationCard(attachment)?.openUrl directly to href without the same safety
check used for attachment.path. Update the rendering around the TooltipTrigger/a
link to validate openUrl with the existing safeLinkURL logic (or a
shared/generalized helper) before assigning it to href, and only render the
active link when the URL passes validation.
In `@frontend/lib/preferences-utils.ts`:
- Around line 65-81: The manually maintained preferenceKeys list can drift from
LocationViewPreferences/DEFAULT_PREFERENCES and silently miss new settings.
Update the logic in preferences-utils.ts so preferenceKeys is derived from
DEFAULT_PREFERENCES (or another single source of truth) instead of a hardcoded
subset, and keep the existing sync behavior in the helper that uses
preferenceKeys so newly added preference fields are included automatically.
- Around line 105-119: The mergeSyncedSettings function currently allowlists
keys via forEachSyncedPreference, but it still copies server-provided values
directly into nextPreferences with only a cast, so malformed shapes can flow
into app state. Add lightweight runtime validation before assigning each synced
value, especially for structurally complex preference keys like
duplicateSettings, quickActions, and tableHeaders. Keep the existing key
allowlist, but gate assignment on basic typeof/shape checks or a small schema
helper so only expected value shapes are merged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4a0f01d2-8e6a-49bd-9044-81a7d299617d
📒 Files selected for processing (25)
backend/app/api/handlers/v1/controller.gobackend/app/api/handlers/v1/v1_ctrl_entities_attachments_external.gobackend/app/api/handlers/v1/v1_ctrl_integration_cards.gobackend/app/api/handlers/v1/v1_ctrl_integration_cards_test.gobackend/app/api/routes.gobackend/internal/core/services/service_items_attachments.gobackend/internal/core/services/service_items_attachments_external_test.gobackend/internal/core/services/service_user.gobackend/internal/data/repo/repo_item_attachments.gobackend/internal/data/repo/repo_item_attachments_test.gofrontend/components/Entity/CreateModal.vuefrontend/components/Item/AttachmentsList.vuefrontend/components/Item/View/ItemChangeDetails.vuefrontend/components/Item/View/table/data-table-dropdown.vuefrontend/composables/use-preferences.test.tsfrontend/composables/use-preferences.tsfrontend/lib/api/base/base-api.tsfrontend/lib/api/classes/items.tsfrontend/lib/preferences-utils.tsfrontend/locales/de.jsonfrontend/locales/en.jsonfrontend/pages/item/[id]/index.vuefrontend/pages/item/[id]/index/edit.vuefrontend/pages/profile.vuefrontend/pages/reports/label-generator.vue
✅ Files skipped from review due to trivial changes (2)
- frontend/lib/api/base/base-api.ts
- frontend/locales/en.json
🚧 Files skipped from review as they are similar to previous changes (9)
- frontend/components/Item/View/ItemChangeDetails.vue
- frontend/composables/use-preferences.test.ts
- frontend/pages/reports/label-generator.vue
- frontend/components/Entity/CreateModal.vue
- frontend/pages/item/[id]/index.vue
- frontend/composables/use-preferences.ts
- backend/internal/data/repo/repo_item_attachments_test.go
- backend/internal/core/services/service_user.go
- backend/internal/core/services/service_items_attachments_external_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/app/api/handlers/v1/v1_ctrl_integration_cards_test.go`:
- Around line 132-136: The test table in v1_ctrl_integration_cards_test.go still
hardcodes the fallback MIME type literal instead of reusing the extracted
thumbnail MIME constants. Update the cases in the integration cards test table
to reference the same production constants used by the thumbnail MIME handling
logic, especially the default/fallback value, so the expectations stay aligned
with the symbols that define MIME normalization.
In `@backend/app/api/handlers/v1/v1_ctrl_integration_cards.go`:
- Around line 82-90: The thumbnail handling in v1_ctrl_integration_cards is
duplicating the MIME literals "application/octet-stream" and "image/png", which
triggers goconst. Introduce shared constants for the thumbnail fallback and
allowed MIME values near the existing integration card helpers (for example
alongside integrationCardBuildTimeout and newPaperlessCardCache), then update
the relevant thumbnail logic and the matching tests to reference those constants
instead of repeating string literals. Use the existing thumbnail-related
functions and helpers in this file to locate all repeated checks and fallback
assignments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9e6903fd-6424-41cb-af61-e92b06becb5e
📒 Files selected for processing (4)
backend/app/api/handlers/v1/v1_ctrl_integration_cards.gobackend/app/api/handlers/v1/v1_ctrl_integration_cards_test.gofrontend/components/Item/AttachmentsList.vuefrontend/pages/profile.vue
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/pages/profile.vue
- frontend/components/Item/AttachmentsList.vue
Comment on lines
+82
to
+90
| const integrationCardBuildTimeout = 15 * time.Second | ||
|
|
||
| func newPaperlessCardCache() *paperlessCardCache { | ||
| return &paperlessCardCache{ | ||
| correspondents: make(map[int]cachedIntegrationLabel), | ||
| documentTypes: make(map[int]cachedIntegrationLabel), | ||
| tags: make(map[int]cachedIntegrationTag), | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Extract thumbnail MIME literals to constants to keep CI green.
goconst is failing on repeated "application/octet-stream" and "image/png" literals. Centralize the thumbnail fallback and allow-list values, then reuse them in the tests.
🔧 Proposed fix
-const integrationCardBuildTimeout = 15 * time.Second
+const (
+ integrationCardBuildTimeout = 15 * time.Second
+
+ thumbnailFallbackContentType = "application/octet-stream"
+ thumbnailAVIFContentType = "image/avif"
+ thumbnailBMPContentType = "image/bmp"
+ thumbnailGIFContentType = "image/gif"
+ thumbnailJPEGContentType = "image/jpeg"
+ thumbnailPNGContentType = "image/png"
+ thumbnailWebPContentType = "image/webp"
+) func thumbnailContentType(contentType string) string {
mediaType, _, err := mime.ParseMediaType(contentType)
if err != nil {
- return "application/octet-stream"
+ return thumbnailFallbackContentType
}
switch strings.ToLower(mediaType) {
- case "image/avif", "image/bmp", "image/gif", "image/jpeg", "image/png", "image/webp":
+ case thumbnailAVIFContentType, thumbnailBMPContentType, thumbnailGIFContentType, thumbnailJPEGContentType, thumbnailPNGContentType, thumbnailWebPContentType:
return mediaType
default:
- return "application/octet-stream"
+ return thumbnailFallbackContentType
}
}Also applies to: 379-390
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/app/api/handlers/v1/v1_ctrl_integration_cards.go` around lines 82 -
90, The thumbnail handling in v1_ctrl_integration_cards is duplicating the MIME
literals "application/octet-stream" and "image/png", which triggers goconst.
Introduce shared constants for the thumbnail fallback and allowed MIME values
near the existing integration card helpers (for example alongside
integrationCardBuildTimeout and newPaperlessCardCache), then update the relevant
thumbnail logic and the matching tests to reference those constants instead of
repeating string literals. Use the existing thumbnail-related functions and
helpers in this file to locate all repeated checks and fallback assignments.
Source: Linters/SAST tools
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Action performedReviews resumed. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
frontend/package.json (1)
89-89: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeep
vueand@vue/runtime-coreon the same version policy. Pinningvueto an exact patch while leaving@vue/runtime-coreon a caret range lets the two drift on later updates; align them to avoid mismatched installs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/package.json` at line 89, The dependency versions for vue and `@vue/runtime-core` are allowed to drift because vue is pinned to an exact patch while `@vue/runtime-core` still uses a caret range. Update the package.json dependency entries so both packages follow the same version policy, using matching versions in the same dependency block to keep installs aligned and avoid mismatched Vue runtime packages.frontend/components/Profile/IntegrationSettings.vue (1)
57-79: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant
getSettings()call before save.
saveSettings()re-fetches full settings (line 59) immediately afterloadSettings()already did on mount, purely to merge unrelated fields. Caching the initially loaded settings object and merging from that would save a round-trip.♻️ Suggested refactor
+ let baseSettings: Record<string, unknown> = {}; + async function loadSettings() { settings.loading = true; const { data, error } = await api.user.getSettings(); settings.loading = false; if (error || !data?.item) { toast.error(t("errors.api_failure")); return; } const item = data.item as Record<string, unknown>; + baseSettings = item; settings.paperless_url = typeof item.paperless_url === "string" ? item.paperless_url : ""; settings.paperless_token = typeof item.paperless_token === "string" ? item.paperless_token : ""; } async function saveSettings() { settings.saving = true; - const { data: current, error: currentError } = await api.user.getSettings(); - if (currentError) { - settings.saving = false; - toast.error(t("profile.toast.failed_settings_save")); - return; - } - const { error } = await api.user.setSettings({ - ...(current?.item ?? {}), + ...baseSettings, paperless_url: setting("paperless_url"), paperless_token: setting("paperless_token"), });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/components/Profile/IntegrationSettings.vue` around lines 57 - 79, The saveSettings flow is doing a redundant api.user.getSettings() fetch before writing the profile integration values. Update IntegrationSettings.vue so saveSettings() reuses the settings loaded by loadSettings() instead of re-fetching, and merge the existing settings object when calling api.user.setSettings. Use the existing loadSettings/saveSettings functions and the current settings state to locate and adjust the logic, while still handling error and saving state cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@frontend/components/Profile/IntegrationSettings.vue`:
- Around line 57-79: The saveSettings flow is doing a redundant
api.user.getSettings() fetch before writing the profile integration values.
Update IntegrationSettings.vue so saveSettings() reuses the settings loaded by
loadSettings() instead of re-fetching, and merge the existing settings object
when calling api.user.setSettings. Use the existing loadSettings/saveSettings
functions and the current settings state to locate and adjust the logic, while
still handling error and saving state cleanup.
In `@frontend/package.json`:
- Line 89: The dependency versions for vue and `@vue/runtime-core` are allowed to
drift because vue is pinned to an exact patch while `@vue/runtime-core` still uses
a caret range. Update the package.json dependency entries so both packages
follow the same version policy, using matching versions in the same dependency
block to keep installs aligned and avoid mismatched Vue runtime packages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 37a3a4fb-de03-43b9-9a0e-84b87ee9b3e8
⛔ Files ignored due to path filters (1)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
backend/app/api/handlers/v1/controller.gobackend/internal/core/services/service_items_attachments_external_test.gobackend/internal/data/repo/repo_item_attachments_test.gofrontend/components/Item/AttachmentsList.vuefrontend/components/Item/IntegrationAttachmentCard.vuefrontend/components/Profile/IntegrationSettings.vuefrontend/lib/api/__test__/user/templates.test.tsfrontend/locales/en.jsonfrontend/package.jsonfrontend/pages/profile.vue
💤 Files with no reviewable changes (2)
- backend/internal/data/repo/repo_item_attachments_test.go
- backend/internal/core/services/service_items_attachments_external_test.go
✅ Files skipped from review due to trivial changes (1)
- frontend/locales/en.json
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/app/api/handlers/v1/controller.go
- frontend/lib/api/test/user/templates.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/package.json (1)
27-27: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winExact-pinning
vue/@vue/runtime-core— consistent, but watch patch-update cadence.Pinning both to the same exact
3.5.38is correct since these Vue core packages must stay version-locked together, and this matches the resolvedpnpm-lock.yamlentries. Security-wise, exact pins (vs. the prior^3.5.38caret range) mean future 3.5.x patch releases — including any security fixes — won't be picked up automatically onpnpm install; ensure there's a process (e.g. Renovate/Dependabot) to bump this pin promptly when Vue ships patch releases.Also applies to: 89-89
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/package.json` at line 27, The Vue core dependencies are now exact-pinned, so patch releases will not be picked up automatically. Keep the version lock between the vue and `@vue/runtime-core` entries in frontend/package.json, and make sure there is an automated or explicit update process to bump both pins together whenever a new 3.5.x patch release is available.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@frontend/package.json`:
- Line 27: The Vue core dependencies are now exact-pinned, so patch releases
will not be picked up automatically. Keep the version lock between the vue and
`@vue/runtime-core` entries in frontend/package.json, and make sure there is an
automated or explicit update process to bump both pins together whenever a new
3.5.x patch release is available.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 66ee69ae-700c-43ef-8d17-9ab88bfcce56
⛔ Files ignored due to path filters (1)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
frontend/package.json
szaiser
force-pushed
the
feat/paperless-immich-attachement-links
branch
from
0153041 to
43a52a7
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/package.json (1)
89-89: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueCaret range loosens version pinning — minor supply-chain consideration.
Switching
vuefrom an exact3.5.20to^3.5.38allows any future3.xminor/patch to be installed on a freshpnpm install, bypassing the lockfile if it's ever regenerated. Since the lockfile currently resolves to3.5.38, this is safe today, but as a general security hygiene note: prefer running CI/deploy installs withpnpm install --frozen-lockfile(if not already the case) so an unreviewed upstream Vue release can't silently slip into the build.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/package.json` at line 89, The Vue dependency in package.json now uses a caret range, which can allow newer 3.x releases to be picked up on lockfile regeneration. Keep the current version decision as-is, but make sure the install pipeline for the frontend uses pnpm install --frozen-lockfile so only the reviewed lockfile-resolved Vue version is built; check the existing CI/deploy install steps and update the package install command if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@frontend/package.json`:
- Line 89: The Vue dependency in package.json now uses a caret range, which can
allow newer 3.x releases to be picked up on lockfile regeneration. Keep the
current version decision as-is, but make sure the install pipeline for the
frontend uses pnpm install --frozen-lockfile so only the reviewed
lockfile-resolved Vue version is built; check the existing CI/deploy install
steps and update the package install command if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 96e935ed-ee3c-4215-8f16-981cea23c768
⛔ Files ignored due to path filters (1)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
frontend/package.json
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
3 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.
Compacted conversation---
What type of PR is this?
What this PR does / why we need it:
Builds on #1481 to add a Paperless-ngx integration and lay the groundwork for further service integrations (e.g. Immich). Users can drag-drop a Paperless document URL onto an item's attachment zone — the document is stored as an external reference (no file copy) and renders as a rich card with thumbnail, title, correspondent, document type, tags, page count and an open-in-Paperless button.
homebox_paperless_link.mp4
Changes:
GET /v1/integrations/{name}/proxy?path={relPath}— generic authenticated reverse-proxy; reads{name}_url/{name}_tokenfrom user settings, validates name and path, forwardsAuthorization: Token {token}MimeTypePaperlessDocument = "paperless/document"registered inMimeTypeForSourceType()integration-adapters.tsregistry (ServiceAdapterinterface +SERVICE_ADAPTERS);integration-cache.tsPinia store for URL state and enriched-data cache; Paperless card inAttachmentsList.vue(thumbnail, metadata, ⚠ error badge); drag-drop URL detection inedit.vue; Paperless URL + token fields inprofile.vueintegration-adapters.test.tsDesign decisions:
SERVICE_ADAPTERSentry on the frontend and one line inMimeTypeForSourceType()on the backend — no further structural changes.link/urlattachments from feat: add support for external URL attachments #1481 whose host matches the configured Paperless URL are auto-promoted to the rich card at render time — fully backward compatibleif/elsechains in drop detection, URL classification, and card hydrationWhich issue(s) this PR fixes:
Testing
npx vitest run lib/integration-adapters.test.ts— 25/25 passgo test ./internal/core/services ./internal/data/repo ./app/api/handlers/v1pass