mirror of
https://github.com/penpot/penpot.git
synced 2026-08-23 21:28:38 +00:00
Merge remote-tracking branch 'origin/staging' into develop
This commit is contained in:
commit
64a52d6b04
@ -3,60 +3,14 @@ Act as a senior software engineer and perform a thorough review.
|
||||
## Instructions
|
||||
|
||||
1. **Determine what is being reviewed** from the provided context:
|
||||
- **If it is a plan** (implementation plan, design document, task breakdown) → follow the **Plan Review** path below.
|
||||
- **If it is code** (diff, PR, code change) → follow the **Code Review** path below.
|
||||
- **If it is a plan** (implementation plan, design document, task breakdown) → load the **`plan-review`** skill.
|
||||
- **If it is code** (diff, PR, code change) → load the **`code-review`** skill.
|
||||
|
||||
---
|
||||
2. Read `AGENTS.md` and follow its instructions for finding and reading all related testing documentation from memories before reviewing.
|
||||
|
||||
## Code Review Path
|
||||
3. **Skip generated files, lockfile-only changes, and unrelated modifications** unless they introduce security risks.
|
||||
|
||||
1. Load the **`code-review-and-quality`** skill — it defines the five axes, core principles (DRY, KISS, YAGNI), severity taxonomy, and output format.
|
||||
2. Read `AGENTS.md` and follow its instructions for finding and reading all related testing documentation from memories before reviewing the code.
|
||||
3. Determine the diff or code to review from the provided context.
|
||||
4. **Skip generated files, lockfile-only changes, and unrelated modifications** unless they introduce security risks.
|
||||
5. Read the diff and the surrounding context for each changed file.
|
||||
6. Review across all five axes: correctness, readability, architecture, security, performance.
|
||||
7. Produce the review using the **Code Review Format** below.
|
||||
8. For each finding:
|
||||
- State the severity (Critical / High / Medium / Low / Suggestion)
|
||||
- Identify the file and line
|
||||
- Describe failure circumstances
|
||||
- **For Critical/High**: Provide a concrete fix with a code snippet showing the corrected code
|
||||
- **For Medium/Low**: Describe the fix clearly; code snippet optional
|
||||
- If multiple approaches exist, briefly note trade-offs
|
||||
9. **Perform a second review pass if the change is complex:**
|
||||
- **Complex indicators**: Critical/High findings, multiple files (>5), architectural changes, security-sensitive code, >300 lines changed
|
||||
- **Skip for simple changes**: Typo fixes, formatting, small bug fixes (<50 lines), single-file changes with no findings
|
||||
- Second pass checks:
|
||||
- Validate severity assignments: Are Critical/High findings truly blockers?
|
||||
- Catch missed issues: Edge cases, error paths, test gaps overlooked in first pass
|
||||
- Remove false positives: Discard findings that aren't real issues
|
||||
- Verify fixes: Are the proposed solutions actually correct and complete?
|
||||
|
||||
---
|
||||
|
||||
## Plan Review Path
|
||||
|
||||
1. Load the **`plan-review`** skill — it defines the six axes, severity taxonomy, and output format.
|
||||
2. Read the full plan from the provided context.
|
||||
3. Review across all six axes: completeness, task quality, architecture & sequencing, risk coverage, actionability, and proposed code quality (if the plan includes implementation details).
|
||||
4. Produce the review using the **Plan Review Format** below.
|
||||
5. For each finding:
|
||||
- State the severity (Critical / Required / Nit / Optional / FYI)
|
||||
- Identify the section or task it refers to
|
||||
- Describe the gap or problem
|
||||
- **For Critical/Required**: Propose a concrete fix or addition
|
||||
- **For Nit/Optional**: Describe the improvement; concrete text optional
|
||||
6. **Perform a second review pass if the plan is complex:**
|
||||
- **Complex indicators**: Critical findings, >10 tasks, migrations or breaking changes, security-sensitive features
|
||||
- **Skip for simple plans**: 1–2 tasks, no risks, no code proposals
|
||||
- Second pass checks:
|
||||
- Validate severity assignments
|
||||
- Catch missed gaps: edge cases, missing dependencies, unaddressed risks
|
||||
- Remove false positives
|
||||
- Verify proposed remedies are actionable
|
||||
|
||||
---
|
||||
4. Follow the loaded skill's process and produce its output format.
|
||||
|
||||
## Strong Rules
|
||||
|
||||
@ -64,75 +18,8 @@ Act as a senior software engineer and perform a thorough review.
|
||||
2. Do not modify any code and do not create a commit — this command only reviews.
|
||||
3. Be specific and constructive. "This could be better" is not helpful — explain why and how.
|
||||
4. Prioritize by impact. One structural issue outweighs ten nits.
|
||||
5. Missing tests are an issue, not a suggestion. If tests are missing or inadequate for new functionality, report it as a severity-tagged finding in the findings sections below — High severity (code) or Required (plan) — never as a recommendation.
|
||||
5. Missing tests are an issue, not a suggestion. Report as a severity-tagged finding — never as a recommendation.
|
||||
|
||||
## Context
|
||||
|
||||
$ARGUMENTS
|
||||
|
||||
## Expected Format — Code Review
|
||||
|
||||
```
|
||||
## Review Summary
|
||||
[1-2 sentences on what the change does and overall assessment]
|
||||
|
||||
## Critical/High Findings
|
||||
|
||||
### [Severity] file.ts:123
|
||||
**Issue**: [Description of the problem]
|
||||
**Impact**: [What could go wrong if this is not fixed]
|
||||
**Fix**:
|
||||
|
||||
````[language]
|
||||
// Current code
|
||||
[problematic code]
|
||||
|
||||
// Fixed code
|
||||
[corrected code]
|
||||
[Optional: note trade-offs if multiple approaches exist]
|
||||
````
|
||||
|
||||
### [Severity] file.ts:456
|
||||
**Issue**: [Description of the problem]
|
||||
**Impact**: [What could go wrong if this is not fixed]
|
||||
**Fix**: [Clear description of the fix; code snippet if it clarifies]
|
||||
|
||||
## Other Findings
|
||||
|
||||
### [Severity] file.ts:789
|
||||
**Issue**: [Description]
|
||||
**Impact**: [Minor consequence or risk]
|
||||
**Fix**: [Clear description; code snippet optional]
|
||||
|
||||
## Positive Observations
|
||||
[2-3 specific things done well]
|
||||
|
||||
## Verdict
|
||||
[Approve / Request Changes / Needs Discussion]
|
||||
[If Request Changes: list the must-fix items]
|
||||
```
|
||||
|
||||
## Expected Format — Plan Review
|
||||
|
||||
```
|
||||
## Review Summary
|
||||
[1-2 sentences on the plan's goal and overall assessment]
|
||||
|
||||
## Critical/Required Findings
|
||||
### [Severity] [Section or Task N]
|
||||
**Issue**: [Description of the gap or problem]
|
||||
**Impact**: [What could go wrong during implementation]
|
||||
**Proposed fix**: [Concrete addition or change to the plan]
|
||||
|
||||
## Other Findings
|
||||
### [Severity] [Section or Task N]
|
||||
**Issue**: [Description]
|
||||
**Proposed fix**: [Clear description; concrete text optional]
|
||||
|
||||
## Strengths
|
||||
[2-3 specific things done well in the plan]
|
||||
|
||||
## Verdict
|
||||
[Approve / Request Changes / Needs Discussion]
|
||||
[If Request Changes: list the must-fix items]
|
||||
```
|
||||
|
||||
@ -1,5 +1,5 @@
|
||||
---
|
||||
name: code-review-and-quality
|
||||
name: code-review
|
||||
description: Conducts multi-axis code review. Use before merging any change. Use when reviewing code written by yourself, another agent, or a human. Use when you need to assess code quality across multiple dimensions before it enters the main branch.
|
||||
---
|
||||
|
||||
@ -87,7 +87,7 @@ Can an implementer actually execute this?
|
||||
|
||||
### 6. Proposed Code Quality *(when the plan includes implementation details)*
|
||||
|
||||
If the plan proposes code shapes, function signatures, data structures, or API designs, evaluate those proposals against `code-review-and-quality` criteria:
|
||||
If the plan proposes code shapes, function signatures, data structures, or API designs, evaluate those proposals against `code-review` criteria:
|
||||
|
||||
- **Correctness:** Do the proposed types/signatures handle edge cases (null, empty, boundaries)?
|
||||
- **Readability:** Are proposed names descriptive and consistent with project conventions?
|
||||
@ -215,7 +215,7 @@ Check that the plan can actually confirm it worked:
|
||||
If the plan includes code snippets, types, or API designs:
|
||||
|
||||
```
|
||||
- Load code-review-and-quality skill for criteria
|
||||
- Load code-review skill for criteria
|
||||
- Check proposed signatures for edge cases
|
||||
- Verify naming follows project conventions
|
||||
- Confirm abstractions follow existing patterns
|
||||
@ -310,6 +310,6 @@ If the plan includes code snippets, types, or API designs:
|
||||
## See Also
|
||||
|
||||
- For producing plans, use the `planner` skill
|
||||
- For reviewing implemented code, use `code-review-and-quality` — also the criteria source for axis 6
|
||||
- For reviewing implemented code, use `code-review` — also the criteria source for axis 6
|
||||
- For security-specific concerns, see `security-and-hardening`
|
||||
- For testing strategy guidance, see `testing`
|
||||
|
||||
@ -723,6 +723,7 @@
|
||||
(-> (select-keys file file-attrs)
|
||||
(assoc :data nil)
|
||||
(dissoc :team-id)
|
||||
(dissoc :metadata)
|
||||
(dissoc :migrations)))
|
||||
|
||||
(defn- file->file-data-params
|
||||
|
||||
@ -392,7 +392,7 @@
|
||||
params {:type "penpot/export-files"
|
||||
:version 1
|
||||
:generated-by (str "penpot/" (:full cf/version))
|
||||
:refer "penpot"
|
||||
:referer "penpot"
|
||||
:files (vec (vals files))
|
||||
:relations rels}]
|
||||
(write-entry! output "manifest.json" params))))
|
||||
@ -734,7 +734,7 @@
|
||||
:plugin-data plugin-data}))
|
||||
|
||||
(defn- import-file
|
||||
[{:keys [::db/conn ::bfc/project-id] :as cfg} {file-id :id file-name :name}]
|
||||
[{:keys [::db/conn ::bfc/project-id ::manifest] :as cfg} {file-id :id file-name :name}]
|
||||
(let [file-id' (bfc/lookup-index file-id)
|
||||
file (read-file cfg file-id)
|
||||
media (read-file-media cfg file-id)
|
||||
@ -801,8 +801,10 @@
|
||||
(assoc :data data)
|
||||
(assoc :name file-name)
|
||||
(assoc :project-id project-id)
|
||||
(assoc :metadata (d/without-nils
|
||||
{:generated-by (get manifest :generated-by)
|
||||
:referer (or (get manifest :referer) (get manifest :refer))}))
|
||||
(dissoc :options))
|
||||
|
||||
file (bfc/process-file cfg file)
|
||||
file (ctf/check-file file)]
|
||||
|
||||
|
||||
@ -52,7 +52,7 @@
|
||||
|
||||
:redis-uri "redis://redis/0"
|
||||
|
||||
:file-data-backend "legacy-db"
|
||||
:file-data-backend "db"
|
||||
|
||||
:objects-storage-backend "fs"
|
||||
:objects-storage-fs-directory "assets"
|
||||
|
||||
@ -12,6 +12,7 @@
|
||||
[app.common.logging :as l]
|
||||
[app.common.schema :as sm]
|
||||
[app.common.time :as ct]
|
||||
[app.common.types.file :as ctf]
|
||||
[app.common.types.objects-map :as omap]
|
||||
[app.config :as cf]
|
||||
[app.db :as db]
|
||||
@ -159,15 +160,17 @@
|
||||
:content-type "application/octet-stream"
|
||||
:file-id file-id
|
||||
:id id})
|
||||
metadata {:storage-ref-id (:id sobject)}
|
||||
metadata (-> (:metadata params)
|
||||
(assoc :storage-ref-id (:id sobject)))
|
||||
params (-> params
|
||||
(assoc :metadata metadata)
|
||||
(assoc :data nil))]
|
||||
(upsert-in-database cfg params))
|
||||
|
||||
(= backend "db")
|
||||
(->> (dissoc params :metadata)
|
||||
(upsert-in-database cfg))
|
||||
(let [metadata (dissoc (:metadata params) :storage-ref-id)
|
||||
params (assoc params :metadata metadata)]
|
||||
(upsert-in-database cfg params))
|
||||
|
||||
(= backend "legacy-db")
|
||||
(cond
|
||||
@ -213,18 +216,11 @@
|
||||
[backend]
|
||||
(or backend (cf/get :file-data-backend)))
|
||||
|
||||
(def ^:private schema:metadata
|
||||
[:map {:title "Metadata"}
|
||||
[:storage-ref-id {:optional true} ::sm/uuid]])
|
||||
|
||||
(def decode-metadata-with-schema
|
||||
(sm/decoder schema:metadata sm/json-transformer))
|
||||
|
||||
(defn decode-metadata
|
||||
[metadata]
|
||||
(some-> metadata
|
||||
(db/decode-json-pgobject)
|
||||
(decode-metadata-with-schema)))
|
||||
(ctf/decode-file-metadata)))
|
||||
|
||||
(def ^:private schema:update-params
|
||||
[:map {:closed true}
|
||||
@ -232,7 +228,7 @@
|
||||
[:type [:enum "main" "snapshot" "fragment"]]
|
||||
[:file-id ::sm/uuid]
|
||||
[:backend {:optional true} [:enum "db" "legacy-db" "storage"]]
|
||||
[:metadata {:optional true} [:maybe schema:metadata]]
|
||||
[:metadata {:optional true} ctf/schema:file-metadata]
|
||||
[:data {:optional true} bytes?]
|
||||
[:created-at {:optional true} ::ct/inst]
|
||||
[:modified-at {:optional true} [:maybe ::ct/inst]]
|
||||
|
||||
@ -34,6 +34,12 @@
|
||||
(assoc :request/auth-data (dissoc auth :token))
|
||||
(assoc :frontend/version (or (yreq/get-header request "x-frontend-version") "unknown")))))
|
||||
|
||||
(defn- strip-internal-fields
|
||||
"Remove fields that leak internal implementation details from error
|
||||
response data. Full context is preserved in server-side logs."
|
||||
[data]
|
||||
(dissoc data :state :path :context))
|
||||
|
||||
(defmulti handle-error
|
||||
(fn [cause _ _]
|
||||
(-> cause ex-data :type)))
|
||||
@ -136,6 +142,7 @@
|
||||
(l/error :hint "assertion error" :cause cause)
|
||||
{::yres/status 500
|
||||
::yres/body (-> data
|
||||
(strip-internal-fields)
|
||||
(assoc :type :server-error)
|
||||
(assoc :code :assertion))})))))
|
||||
|
||||
@ -161,9 +168,9 @@
|
||||
(l/error :hint "internal error" :cause cause)
|
||||
{::yres/status 500
|
||||
::yres/body (-> data
|
||||
(strip-internal-fields)
|
||||
(assoc :type :server-error)
|
||||
(update :code #(or % :unhandled))
|
||||
(assoc :hint (ex-message error)))})))
|
||||
(update :code #(or % :unhandled)))})))
|
||||
|
||||
(defmethod handle-error :default
|
||||
[error request parent-cause]
|
||||
@ -178,6 +185,20 @@
|
||||
(handle-exception (:handling edata) request error)
|
||||
(handle-exception error request parent-cause))))
|
||||
|
||||
(defn- pgsql-state->message
|
||||
"Map PostgreSQL SQLSTATE codes to safe, client-facing messages.
|
||||
Returns a user-friendly string that conveys the nature of the error
|
||||
without exposing table names, constraint names, or other internals."
|
||||
[state]
|
||||
(case state
|
||||
"23505" "A conflicting entry already exists"
|
||||
"23503" "The referenced item does not exist"
|
||||
"23502" "A required field is missing"
|
||||
"23514" "The value violates a data integrity constraint"
|
||||
"57014" "The operation took too long and was cancelled"
|
||||
"25P03" "The transaction was idle too long and was cancelled"
|
||||
"A database error occurred"))
|
||||
|
||||
(defmethod handle-exception org.postgresql.util.PSQLException
|
||||
[error request parent-cause]
|
||||
(let [state (.getSQLState ^java.sql.SQLException error)
|
||||
@ -190,20 +211,19 @@
|
||||
{::yres/status 504
|
||||
::yres/body {:type :server-error
|
||||
:code :statement-timeout
|
||||
:hint (ex-message error)}}
|
||||
:hint (pgsql-state->message state)}}
|
||||
|
||||
(= state "25P03")
|
||||
{::yres/status 504
|
||||
::yres/body {:type :server-error
|
||||
:code :idle-in-transaction-timeout
|
||||
:hint (ex-message error)}}
|
||||
:hint (pgsql-state->message state)}}
|
||||
|
||||
:else
|
||||
{::yres/status 500
|
||||
::yres/body {:type :server-error
|
||||
:code :unexpected
|
||||
:hint (ex-message error)
|
||||
:state state}}))))
|
||||
:code :database-error
|
||||
:hint (pgsql-state->message state)}}))))
|
||||
|
||||
(defmethod handle-exception :default
|
||||
[error request parent-cause]
|
||||
@ -216,17 +236,16 @@
|
||||
(l/error :hint "unexpected error" :cause cause)
|
||||
{::yres/status 500
|
||||
::yres/body {:type :server-error
|
||||
:code :unexpected
|
||||
:hint (ex-message error)}})
|
||||
:code :unexpected}})
|
||||
|
||||
:else
|
||||
(binding [l/*context* (request->context request)]
|
||||
(l/error :hint "unhandled error" :cause cause)
|
||||
{::yres/status 500
|
||||
::yres/body (-> edata
|
||||
(strip-internal-fields)
|
||||
(assoc :type :server-error)
|
||||
(update :code #(or % :unhandled))
|
||||
(assoc :hint (ex-message error)))}))))
|
||||
(update :code #(or % :unhandled)))}))))
|
||||
|
||||
(defmethod handle-exception java.io.IOException
|
||||
[cause request _]
|
||||
@ -234,9 +253,7 @@
|
||||
(l/wrn :hint "io exception" :cause cause)
|
||||
{::yres/status 500
|
||||
::yres/body {:type :server-error
|
||||
:code :io-exception
|
||||
:hint (ex-message cause)
|
||||
:path (:path request)}}))
|
||||
:code :io-exception}}))
|
||||
|
||||
(defmethod handle-exception java.util.concurrent.CompletionException
|
||||
[cause request _]
|
||||
|
||||
@ -83,18 +83,18 @@
|
||||
(instance? IllegalArgumentException cause)
|
||||
(ex/raise :type :validation
|
||||
:code :malformed-json
|
||||
:hint (ex-message cause)
|
||||
:hint "invalid JSON in request body"
|
||||
:cause cause)
|
||||
|
||||
(instance? RequestTooBigException cause)
|
||||
(ex/raise :type :validation
|
||||
:code :request-body-too-large
|
||||
:hint (ex-message cause))
|
||||
:hint "request body exceeds size limit")
|
||||
|
||||
(instance? java.io.EOFException cause)
|
||||
(ex/raise :type :validation
|
||||
:code :malformed-json
|
||||
:hint (ex-message cause)
|
||||
:hint "unexpected end of request body"
|
||||
:cause cause)
|
||||
|
||||
(instance? RuntimeException cause)
|
||||
|
||||
@ -204,7 +204,7 @@
|
||||
[{:keys [::manager]}]
|
||||
(assert (manager? manager) "expected valid session manager")
|
||||
(fn [request response]
|
||||
(some->> (get request ::id) (delete-session manager))
|
||||
(some->> (get request ::session) :id (delete-session manager))
|
||||
(clear-session-cookie response)))
|
||||
|
||||
(defn decode-token
|
||||
|
||||
@ -221,6 +221,29 @@
|
||||
(t/is (str/starts-with? (str uri)
|
||||
"https://example.com/penpot/assets/by-id/"))))
|
||||
|
||||
(t/deftest import-binfile-v3-persists-manifest-metadata
|
||||
(let [profile (th/create-profile* 1)
|
||||
file (prepare-simple-file profile)
|
||||
output (tmp/tempfile :suffix ".zip")]
|
||||
|
||||
(v3/export-files!
|
||||
(-> th/*system*
|
||||
(assoc ::bfc/ids #{(:id file)})
|
||||
(assoc ::bfc/embed-assets false)
|
||||
(assoc ::bfc/include-libraries false))
|
||||
(io/output-stream output))
|
||||
|
||||
(let [result (-> th/*system*
|
||||
(assoc ::bfc/project-id (:default-project-id profile))
|
||||
(assoc ::bfc/profile-id (:id profile))
|
||||
(assoc ::bfc/input output)
|
||||
(v3/import-files!))
|
||||
imported (bfc/get-file th/*system* (first result))]
|
||||
|
||||
(t/is (= (count result) 1))
|
||||
(t/is (some? (get-in imported [:metadata :generated-by])))
|
||||
(t/is (= "penpot" (get-in imported [:metadata :referer]))))))
|
||||
|
||||
(t/deftest read-obj-rejects-oversized-buffer
|
||||
;; N1-07: read-obj! must reject objects exceeding max-object-size
|
||||
;; before attempting to allocate the buffer
|
||||
|
||||
@ -6,10 +6,12 @@
|
||||
|
||||
(ns backend-tests.http-middleware-test
|
||||
(:require
|
||||
[app.common.exceptions :as ex]
|
||||
[app.common.time :as ct]
|
||||
[app.db :as db]
|
||||
[app.http :as-alias http]
|
||||
[app.http.access-token]
|
||||
[app.http.errors :as http-errors]
|
||||
[app.http.middleware :as mw]
|
||||
[app.http.session :as session]
|
||||
[app.main :as-alias main]
|
||||
@ -300,7 +302,7 @@
|
||||
(t/is (instance? clojure.lang.ExceptionInfo ex))
|
||||
(t/is (= :validation (-> ex ex-data :type)))
|
||||
(t/is (= :malformed-json (-> ex ex-data :code)))
|
||||
(t/is (string? (-> ex ex-data :hint)))))
|
||||
(t/is (= "invalid JSON in request body" (-> ex ex-data :hint)))))
|
||||
|
||||
(t/deftest parse-request-request-too-big-exception
|
||||
;; When RequestTooBigException is raised (e.g. the request body
|
||||
@ -319,7 +321,7 @@
|
||||
(t/is (instance? clojure.lang.ExceptionInfo ex))
|
||||
(t/is (= :validation (-> ex ex-data :type)))
|
||||
(t/is (= :request-body-too-large (-> ex ex-data :code)))
|
||||
(t/is (string? (-> ex ex-data :hint)))))
|
||||
(t/is (= "request body exceeds size limit" (-> ex ex-data :hint)))))
|
||||
|
||||
(t/deftest parse-request-eof-exception
|
||||
;; When java.io.EOFException is raised (e.g. the body stream
|
||||
@ -337,7 +339,7 @@
|
||||
(t/is (instance? clojure.lang.ExceptionInfo ex))
|
||||
(t/is (= :validation (-> ex ex-data :type)))
|
||||
(t/is (= :malformed-json (-> ex ex-data :code)))
|
||||
(t/is (string? (-> ex ex-data :hint)))))
|
||||
(t/is (= "unexpected end of request body" (-> ex ex-data :hint)))))
|
||||
|
||||
(t/deftest parse-request-runtime-exception-with-cause
|
||||
;; When a RuntimeException with a non-nil ex-cause is raised,
|
||||
@ -377,7 +379,7 @@
|
||||
(t/is (= 500 (::yres/status response)))
|
||||
(t/is (= :server-error (:type body)))
|
||||
(t/is (= :unexpected (:code body)))
|
||||
(t/is (= "boom" (:hint body)))))
|
||||
(t/is (nil? (:hint body)))))
|
||||
|
||||
(t/deftest parse-request-non-runtime-throwable
|
||||
;; When a non-RuntimeException Throwable is raised (e.g. an
|
||||
@ -397,4 +399,45 @@
|
||||
(t/is (= 500 (::yres/status response)))
|
||||
(t/is (= :server-error (:type body)))
|
||||
(t/is (= :io-exception (:code body)))
|
||||
(t/is (= "network gone" (:hint body)))))
|
||||
(t/is (nil? (:hint body)))))
|
||||
|
||||
(t/deftest internal-error-strips-sensitive-fields
|
||||
;; When an :internal error is raised with :state, :path, and
|
||||
;; :context, those fields must not appear in the response body.
|
||||
;; :hint is part of the error protocol and is preserved.
|
||||
(let [cause (ex-info "internal error"
|
||||
{:type :internal
|
||||
:code :test-error
|
||||
:hint "safe user-facing hint"
|
||||
:state "XX000"
|
||||
:path "/data/penpot/storage"
|
||||
:context {:backend :s3 :bucket "prod"}})
|
||||
response (http-errors/handle cause {})
|
||||
body (::yres/body response)]
|
||||
(t/is (= 500 (::yres/status response)))
|
||||
(t/is (= :server-error (:type body)))
|
||||
(t/is (= :test-error (:code body)))
|
||||
(t/is (= "safe user-facing hint" (:hint body)))
|
||||
(t/is (nil? (:state body)))
|
||||
(t/is (nil? (:path body)))
|
||||
(t/is (nil? (:context body)))))
|
||||
|
||||
(t/deftest unhandled-exinfo-strips-sensitive-fields
|
||||
;; When an ex-info with an unregistered :type (dispatches through
|
||||
;; handle-exception :default :else) carries :state and :path,
|
||||
;; those fields must not appear in the response body.
|
||||
;; :hint is part of the error protocol and is preserved.
|
||||
(let [cause (ex-info "something broke"
|
||||
{:type :unregistered-type
|
||||
:code :custom-code
|
||||
:hint "safe user-facing hint"
|
||||
:state "internal-state"
|
||||
:path "/internal/path"})
|
||||
response (http-errors/handle cause {})
|
||||
body (::yres/body response)]
|
||||
(t/is (= 500 (::yres/status response)))
|
||||
(t/is (= :server-error (:type body)))
|
||||
(t/is (= :custom-code (:code body)))
|
||||
(t/is (= "safe user-facing hint" (:hint body)))
|
||||
(t/is (nil? (:state body)))
|
||||
(t/is (nil? (:path body)))))
|
||||
|
||||
106
backend/test/backend_tests/rpc_auth_test.clj
Normal file
106
backend/test/backend_tests/rpc_auth_test.clj
Normal file
@ -0,0 +1,106 @@
|
||||
;; This Source Code Form is subject to the terms of the Mozilla Public
|
||||
;; License, v. 2.0. If a copy of the MPL was not distributed with this
|
||||
;; file, You can obtain one at http://mozilla.org/MPL/2.0/.
|
||||
;;
|
||||
;; Copyright (c) KALEIDOS INC Sucursal en España SL
|
||||
|
||||
(ns backend-tests.rpc-auth-test
|
||||
(:require
|
||||
[app.common.uuid :as uuid]
|
||||
[app.http.session :as session]
|
||||
[backend-tests.helpers :as th]
|
||||
[clojure.test :as t]
|
||||
[yetti.response :as yres]))
|
||||
|
||||
(t/use-fixtures :once th/state-init)
|
||||
(t/use-fixtures :each th/database-reset)
|
||||
|
||||
(t/deftest logout-invalidates-current-session
|
||||
(let [prof (th/create-profile* 1)
|
||||
manager (::session/manager th/*system*)
|
||||
sid (uuid/random)
|
||||
_ (th/db-exec-one! ["INSERT INTO http_session_v2 (id, profile_id, user_agent) VALUES (?, ?, ?)"
|
||||
sid (:id prof) "test-agent"])
|
||||
session (session/read-session manager sid)]
|
||||
|
||||
;; Arrange: session exists before logout
|
||||
(t/is (some? session) "session should exist before logout")
|
||||
(t/is (= sid (:id session)))
|
||||
|
||||
;; Act: simulate Ring request as produced by wrap-authz (has ::session/session)
|
||||
;; delete-fn is used as response transform via rph/with-transform in auth/logout
|
||||
(let [request {::session/session session}
|
||||
response {}
|
||||
delete-fn (session/delete-fn th/*system*)
|
||||
result (delete-fn request response)]
|
||||
|
||||
;; Assert: server-side session is deleted (CWE-613)
|
||||
(t/is (nil? (session/read-session manager sid))
|
||||
"session must be deleted server-side after logout (GHSA-mj9f-5cwq-7p3q)")
|
||||
|
||||
;; Assert: cookie is cleared
|
||||
(t/is (= "" (get-in result [::yres/cookies "auth-token" :value]))
|
||||
"auth-token cookie should be cleared")
|
||||
(t/is (= 0 (get-in result [::yres/cookies "auth-token" :max-age]))
|
||||
"auth-token cookie max-age should be 0"))))
|
||||
|
||||
(t/deftest logout-clears-cookie-even-when-session-missing
|
||||
(let [manager (::session/manager th/*system*)
|
||||
sid (uuid/random)
|
||||
;; No session inserted, read should be nil
|
||||
_ (t/is (nil? (session/read-session manager sid)))
|
||||
request {}
|
||||
response {}
|
||||
delete-fn (session/delete-fn th/*system*)
|
||||
result (delete-fn request response)]
|
||||
|
||||
;; Should still clear cookie (idempotent)
|
||||
(t/is (= "" (get-in result [::yres/cookies "auth-token" :value])))
|
||||
(t/is (= 0 (get-in result [::yres/cookies "auth-token" :max-age])))))
|
||||
|
||||
(t/deftest logout-does-not-invalidate-other-sessions
|
||||
(let [prof (th/create-profile* 1)
|
||||
manager (::session/manager th/*system*)
|
||||
sid1 (uuid/random)
|
||||
sid2 (uuid/random)
|
||||
_ (th/db-exec-one! ["INSERT INTO http_session_v2 (id, profile_id, user_agent) VALUES (?, ?, ?)"
|
||||
sid1 (:id prof) "agent-1"])
|
||||
_ (th/db-exec-one! ["INSERT INTO http_session_v2 (id, profile_id, user_agent) VALUES (?, ?, ?)"
|
||||
sid2 (:id prof) "agent-2"])
|
||||
s1 (session/read-session manager sid1)
|
||||
s2 (session/read-session manager sid2)]
|
||||
|
||||
(t/is (some? s1))
|
||||
(t/is (some? s2))
|
||||
|
||||
;; Logout only sid1
|
||||
(let [request {::session/session s1}
|
||||
response {}
|
||||
delete-fn (session/delete-fn th/*system*)]
|
||||
(delete-fn request response))
|
||||
|
||||
;; sid1 deleted, sid2 intact
|
||||
(t/is (nil? (session/read-session manager sid1)) "current session should be deleted")
|
||||
(t/is (some? (session/read-session manager sid2)) "other sessions should remain")))
|
||||
|
||||
(t/deftest replay-after-logout-cannot-authenticate
|
||||
(let [prof (th/create-profile* 1)
|
||||
manager (::session/manager th/*system*)
|
||||
sid (uuid/random)
|
||||
_ (th/db-exec-one! ["INSERT INTO http_session_v2 (id, profile_id, user_agent) VALUES (?, ?, ?)"
|
||||
sid (:id prof) "test-agent"])
|
||||
session (session/read-session manager sid)]
|
||||
|
||||
(t/is (some? session) "session exists before logout")
|
||||
|
||||
;; Simulate logout
|
||||
(let [request {::session/session session}
|
||||
response {}
|
||||
delete-fn (session/delete-fn th/*system*)]
|
||||
(delete-fn request response))
|
||||
|
||||
;; Replay: attempt to read session with same sid should fail (no profile attached)
|
||||
(t/is (nil? (session/read-session manager sid))
|
||||
"replayed token must not resolve to a valid session after logout")))
|
||||
|
||||
|
||||
@ -88,6 +88,12 @@
|
||||
[:plugin-data {:optional true} schema:plugin-data]
|
||||
[:tokens-lib {:optional true} schema:tokens-lib]])
|
||||
|
||||
(def schema:file-metadata
|
||||
[:map {:title "Metadata"}
|
||||
[:storage-ref-id {:optional true} ::sm/uuid]
|
||||
[:generated-by {:optional true} :string]
|
||||
[:referer {:optional true} :string]])
|
||||
|
||||
(def schema:file
|
||||
"A schema for validate a file data structure; data is optional
|
||||
because sometimes we want to validate file without the data."
|
||||
@ -106,6 +112,7 @@
|
||||
[:data {:optional true} schema:data]
|
||||
[:version :int]
|
||||
[:features ::cfeat/features]
|
||||
[:metadata {:optional true} schema:file-metadata]
|
||||
[:migrations {:optional true}
|
||||
[::sm/set {:ordered true} :string]]])
|
||||
|
||||
@ -123,6 +130,9 @@
|
||||
(def check-file-media
|
||||
(sm/check-fn schema:media))
|
||||
|
||||
(def decode-file-metadata
|
||||
(sm/decoder schema:file-metadata sm/json-transformer))
|
||||
|
||||
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
|
||||
;; INITIALIZATION
|
||||
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
|
||||
|
||||
@ -100,7 +100,7 @@ RUN set -eux; \
|
||||
|
||||
FROM base AS setup-opencode
|
||||
|
||||
ENV OPENCODE_VERSION=1.18.18
|
||||
ENV OPENCODE_VERSION=1.18.19
|
||||
|
||||
RUN set -ex; \
|
||||
ARCH="$(dpkg --print-architecture)"; \
|
||||
|
||||
@ -17,12 +17,14 @@
|
||||
[app.common.geom.proportions :as gpp]
|
||||
[app.common.geom.shapes :as gsh]
|
||||
[app.common.logging :as log]
|
||||
[app.common.math :as mth]
|
||||
[app.common.path-names :as cpn]
|
||||
[app.common.render-wasm.wasm :as wasm-state]
|
||||
[app.common.transit :as t]
|
||||
[app.common.types.component :as ctc]
|
||||
[app.common.types.components-list :as ctkl]
|
||||
[app.common.types.shape :as cts]
|
||||
[app.common.types.tokens-lib :as ctob]
|
||||
[app.common.types.variant :as ctv]
|
||||
[app.common.uuid :as uuid]
|
||||
[app.config :as cf]
|
||||
@ -266,6 +268,59 @@
|
||||
(rx/map (fn [_] (mcp/init))))
|
||||
(rx/empty))))))
|
||||
|
||||
(defn- compute-shape-stats
|
||||
"Compute shape statistics in a single pass over pages-index.
|
||||
Returns {:num-shapes N :max-shapes-per-page M}"
|
||||
[pages-index]
|
||||
(reduce-kv
|
||||
(fn [acc _page-id page]
|
||||
(let [n (count (:objects page))]
|
||||
(-> acc
|
||||
(update :num-shapes + n)
|
||||
(update :max-shapes-per-page max n))))
|
||||
{:num-shapes 0
|
||||
:max-shapes-per-page 0}
|
||||
pages-index))
|
||||
|
||||
(defn compute-file-stats
|
||||
"Compute file statistics. Returns a map of stats without event keys."
|
||||
[state file-id]
|
||||
(let [file (dsh/lookup-file state file-id)
|
||||
file-data (:data file)
|
||||
libraries (refs/select-libraries (:files state) file-id)
|
||||
pages-index (:pages-index file-data)
|
||||
{:keys [num-shapes max-shapes-per-page]} (compute-shape-stats pages-index)
|
||||
n-pages (count (:pages file-data))
|
||||
n-components (reduce-kv (fn [n _ c] (if (:deleted c) n (inc n)))
|
||||
0 (:components file-data))
|
||||
n-linked-libs (dec (count libraries))
|
||||
tokens-lib (:tokens-lib file-data)
|
||||
n-tokens (if (some? tokens-lib)
|
||||
(count (ctob/get-all-tokens tokens-lib))
|
||||
0)]
|
||||
{:num-pages n-pages
|
||||
:num-shapes num-shapes
|
||||
:avg-shapes-per-page (if (pos? n-pages)
|
||||
(mth/round (/ num-shapes n-pages))
|
||||
0)
|
||||
:max-shapes-per-page max-shapes-per-page
|
||||
:num-components n-components
|
||||
:num-linked-libraries (max 0 n-linked-libs)
|
||||
:is-library (:is-shared file)
|
||||
:num-tokens n-tokens}))
|
||||
|
||||
(defn- emit-workspace-file-stats
|
||||
[file-id team-id]
|
||||
(ptk/reify ::emit-workspace-file-stats
|
||||
ptk/WatchEvent
|
||||
(watch [_ state _]
|
||||
(let [stats (compute-file-stats state file-id)]
|
||||
(rx/of (ev/event (assoc stats
|
||||
::ev/name "open-workspace-file"
|
||||
::ev/origin "workspace"
|
||||
:file-id file-id
|
||||
:team-id team-id)))))))
|
||||
|
||||
(defn- bundle-fetched
|
||||
[{:keys [file file-id thumbnails] :as bundle}]
|
||||
(ptk/reify ::bundle-fetched
|
||||
@ -421,6 +476,12 @@
|
||||
(rx/take 1)
|
||||
(rx/map dwc/set-workspace-visited))
|
||||
|
||||
;; Emit audit event with file statistics once all libraries are resolved
|
||||
(->> stream
|
||||
(rx/filter (ptk/type? ::all-libraries-resolved))
|
||||
(rx/take 1)
|
||||
(rx/map #(emit-workspace-file-stats file-id team-id)))
|
||||
|
||||
(when-let [component-id (some-> rparams :component-id uuid/parse)]
|
||||
(->> stream
|
||||
(rx/filter (ptk/type? ::workspace-initialized))
|
||||
|
||||
87
frontend/test/frontend_tests/data/workspace_stats_test.cljs
Normal file
87
frontend/test/frontend_tests/data/workspace_stats_test.cljs
Normal file
@ -0,0 +1,87 @@
|
||||
;; This Source Code Form is subject to the terms of the Mozilla Public
|
||||
;; License, v. 2.0. If a copy of the MPL was not distributed with this
|
||||
;; file, You can obtain one at http://mozilla.org/MPL/2.0/.
|
||||
;;
|
||||
;; Copyright (c) KALEIDOS INC Sucursal en España SL
|
||||
|
||||
(ns frontend-tests.data.workspace-stats-test
|
||||
(:require
|
||||
[app.common.test-helpers.files :as cthf]
|
||||
[app.common.test-helpers.ids-map :as cthi]
|
||||
[app.common.types.tokens-lib :as ctob]
|
||||
[app.main.data.workspace :as dw]
|
||||
[cljs.test :as t :include-macros true]
|
||||
[frontend-tests.helpers.state :as ths]))
|
||||
|
||||
(t/use-fixtures :each
|
||||
{:before cthi/reset-idmap!})
|
||||
|
||||
;; ---------------------------------------------------------------------------
|
||||
;; Test compute-file-stats with various edge cases
|
||||
;; ---------------------------------------------------------------------------
|
||||
|
||||
(t/deftest compute-file-stats-empty-file
|
||||
(t/testing "empty file with no pages"
|
||||
(let [file (cthf/sample-file :file1 :page-label :page1)
|
||||
store (ths/setup-store file)
|
||||
state @store
|
||||
file-id (:id file)
|
||||
stats (dw/compute-file-stats state file-id)]
|
||||
(t/is (= (:num-pages stats) 1))
|
||||
(t/is (>= (:num-shapes stats) 0))
|
||||
(t/is (>= (:avg-shapes-per-page stats) 0))
|
||||
(t/is (>= (:max-shapes-per-page stats) 0))
|
||||
(t/is (>= (:num-components stats) 0))
|
||||
(t/is (>= (:num-linked-libraries stats) 0))
|
||||
(t/is (boolean? (:is-library stats)))
|
||||
(t/is (>= (:num-tokens stats) 0)))))
|
||||
|
||||
(t/deftest compute-file-stats-with-shapes
|
||||
(t/testing "file with shapes"
|
||||
(let [file (-> (cthf/sample-file :file1 :page-label :page1)
|
||||
(cthf/add-sample-shape :shape1)
|
||||
(cthf/add-sample-shape :shape2))
|
||||
store (ths/setup-store file)
|
||||
state @store
|
||||
file-id (:id file)
|
||||
stats (dw/compute-file-stats state file-id)]
|
||||
(t/is (= (:num-pages stats) 1))
|
||||
(t/is (>= (:num-shapes stats) 2))
|
||||
(t/is (>= (:avg-shapes-per-page stats) 2))
|
||||
(t/is (>= (:max-shapes-per-page stats) 2)))))
|
||||
|
||||
(t/deftest compute-file-stats-no-tokens
|
||||
(t/testing "file with no tokens lib"
|
||||
(let [file (cthf/sample-file :file1 :page-label :page1)
|
||||
store (ths/setup-store file)
|
||||
state @store
|
||||
file-id (:id file)
|
||||
stats (dw/compute-file-stats state file-id)]
|
||||
(t/is (= (:num-tokens stats) 0)))))
|
||||
|
||||
(t/deftest compute-file-stats-with-tokens
|
||||
(t/testing "file with tokens"
|
||||
(let [tokens-lib (-> (ctob/make-tokens-lib)
|
||||
(ctob/add-set {:name "global"
|
||||
:description "Global tokens"
|
||||
:tokens [{:name "color.primary"
|
||||
:type :color
|
||||
:value "#000000"}]}))
|
||||
file (-> (cthf/sample-file :file1 :page-label :page1)
|
||||
(assoc-in [:data :tokens-lib] tokens-lib))
|
||||
store (ths/setup-store file)
|
||||
state @store
|
||||
file-id (:id file)
|
||||
stats (dw/compute-file-stats state file-id)]
|
||||
(t/is (= (:num-tokens stats) 1)))))
|
||||
|
||||
(t/deftest compute-file-stats-multiple-pages
|
||||
(t/testing "file with multiple pages"
|
||||
(let [file (-> (cthf/sample-file :file1 :page-label :page1)
|
||||
(cthf/add-sample-page :page2)
|
||||
(cthf/add-sample-page :page3))
|
||||
store (ths/setup-store file)
|
||||
state @store
|
||||
file-id (:id file)
|
||||
stats (dw/compute-file-stats state file-id)]
|
||||
(t/is (= (:num-pages stats) 3)))))
|
||||
Loading…
x
Reference in New Issue
Block a user