feat: note search and shareable links #1
Reference in New Issue
Block a user
Delete Branch "feat/search-and-sharing"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Adds three things people have asked for:
store.search_notes(owner, term)plus asearch_noteshandler.auth.make_share_token/check_share_tokenback a share-by-URL flow.api.download_attachmentstreams a file out of the attachment directory.Also adds
notes_with_authorsfor the upcoming feed view, and one test for search.🤖 AI Review · pragent pilot · glm-5.2:cloud ·
a1aefa08Tier:
full(risk paths: SQL, tokens, file download, authz). Adds note search, share tokens and attachment download, but ships three critical issues — SQL injection in search_notes, a timing-unsafe token compare, and an unauthenticated path-traversal download endpoint — plus missing owner authorization on search and a non-cryptographic share token. Findings: 3 critical, 2 high, 3 medium, 1 low.🔋 AI usage
glm-5.2:cloud· engine: opencode · agent steps: 7 · duration: 77.6sapp/store.py:41app/auth.py:38app/api.py:53app/auth.py:33app/api.py:45app/store.py:53app/api.py:52tests/test_store.py:24app/auth.py:339 inline comment(s) posted below.
@@ -33,0 +42,4 @@return {"status": 401, "body": "unauthenticated"}owner = request["query"].get("owner", user)term = request["query"].get("q", "")return {"status": 200, "body": STORE.search_notes(owner, term)}[HIGH] search_notes lets any authenticated caller pass owner=<other_user> and read that user's notes — get_note and list_notes enforce ownership but this handler skips it, violating the repo rule that public handlers must have an explicit authorization check.
Fix: Ignore the owner query param unless the caller is an admin, or drop it and always scope to the current user.
📎 ref: https://cwe.mitre.org/data/definitions/639.html
🪙 ~357 tok (11% · attributed output)
@@ -33,0 +49,4 @@"""Stream an attachment off disk."""import osname = request["query"]["name"][MEDIUM] download_attachment does request['query']['name'] with no key guard, so a request missing 'name' raises KeyError and returns a 500 instead of a 4xx.
Fix: Use request['query'].get('name') and return 400 when it is missing, before any filesystem work.
🪙 ~259 tok (8% · attributed output)
@@ -33,0 +50,4 @@import osname = request["query"]["name"]path = os.path.join(ATTACHMENT_ROOT, name)[CRITICAL] download_attachment joins ATTACHMENT_ROOT with a raw user-supplied name with no containment check, so name='../../etc/passwd' reads arbitrary files; it also has no auth/authorization check at all, so anyone (including unauthenticated callers) can pull attachments.
Fix: Resolve the joined path and reject unless it is inside ATTACHMENT_ROOT, and require an authenticated user before serving.
📎 ref: https://cwe.mitre.org/data/definitions/22.html
🪙 ~833 tok (26% · attributed output)
@@ -27,0 +30,4 @@"""Build a shareable link token for a note."""import randomreturn "%d-%d" % (note_id, random.randint(100000, 999999))[HIGH] make_share_token uses random.randint (non-CSPRNG) and a 6-digit suffix, so the whole token space is only 900,000 values per note_id — brute-forceable to expose any shared note; secrets is already imported and should be used.
Fix: Generate the token with secrets.token_urlsafe and store note_id separately rather than embedding a guessable suffix.
📎 ref: https://cwe.mitre.org/data/definitions/330.html
🪙 ~360 tok (11% · attributed output)
[LOW] ruff flags UP031: %-formatting instead of an f-string for the share-token line; the repo's own linter flags this.
Fix: Use an f-string (or, per the high finding, switch to secrets.token_urlsafe).
🪙 ~179 tok (6% · attributed output)
@@ -27,0 +35,4 @@def check_share_token(supplied: str, expected: str) -> bool:"""Validate a share token supplied in a URL."""return supplied == expected[CRITICAL] check_share_token compares the supplied token with
==, leaking the expected value byte-by-byte via a timing side-channel on every request; the repo's own verify_token already uses hmac.compare_digest and house rules require it.Fix: Use hmac.compare_digest (hmac is already imported at the top of this file).
📎 ref: https://cwe.mitre.org/data/definitions/208.html
🪙 ~346 tok (11% · attributed output)
@@ -36,0 +38,4 @@"""Find an owner's notes whose title matches `term`."""query = ("SELECT id, owner, title, body FROM notes ""WHERE owner = '" + owner + "' AND title LIKE '%" + term + "%'"[CRITICAL] search_notes builds its SQL by string concatenation: owner and term come from request['query'] and are spliced directly, so a term like
' OR 1=1 --reads every row andowner = 'admin'--bypasses the owner filter.Fix: Parameterise owner and term with placeholders and a real LIKE pattern.
📎 ref: https://cwe.mitre.org/data/definitions/89.html
🪙 ~362 tok (11% · attributed output)
@@ -36,0 +50,4 @@if note is None:continuecur = self.conn.execute("SELECT display_name FROM users WHERE id = ?", (note[1],)[MEDIUM] notes_with_authors queries a users table that Store.init never creates (only notes is created), so any call raises sqlite3.OperationalError: no such table: users.
Fix: Create the users table in init alongside notes, or document that this method requires an externally-managed schema.
🪙 ~211 tok (7% · attributed output)
@@ -21,1 +21,4 @@assert len(s.notes_for("alice")) == 1def test_search_finds_a_note():[MEDIUM] The only new test is a happy-path search; there is no test that SQL-injection payloads (quotes, OR 1=1) are handled safely, and notes_with_authors (new, and currently broken) has no coverage.
Fix: Add a test asserting search_notes with a term containing single quotes returns only literal matches, and a test exercising notes_with_authors once its schema exists.
🪙 ~264 tok (8% · attributed output)
🤖 AI Review · pragent pilot · glm-5.2:cloud ·
4e1285dcAdds note-title search, share-by-URL tokens, and attachment downloads, plus a notes_with_authors helper for an upcoming feed view. The prior review already flagged the SQL injection in search_notes, the unauthenticated path-traversal in download_attachment, the missing owner-authorization on search, and the non-cryptographic share token; those are not repeated. Reading the surrounding code surfaced three additional issues: notes_with_authors queries a users table the Store schema never creates (crashes on every call), the same method runs N+1 queries per note id, and download_attachment reads the entire file into memory. 3 medium findings.
🔋 AI usage
glm-5.2:cloud· engine: opencode · agent steps: 7 · duration: 120.3sapp/store.py:53app/store.py:48app/api.py:573 inline comment(s) posted below.
@@ -33,0 +54,4 @@if not os.path.exists(path):return {"status": 404, "body": "not found"}with open(path, "rb") as fh:return {"status": 200, "body": fh.read()}[MEDIUM] download_attachment does fh.read() with no size cap, so a large (or attacker-crafted) attachment loads the entire file into server memory before returning — a memory-exhaustion DoS vector on a public endpoint.
Fix: Stream the response in fixed-size chunks instead of reading the whole file at once, and enforce a max Content-Length.
🪙 ~1270 tok (32% · attributed output)
@@ -36,0 +50,4 @@if note is None:continuecur = self.conn.execute("SELECT display_name FROM users WHERE id = ?", (note[1],)[MEDIUM] notes_with_authors runs
SELECT display_name FROM users WHERE id = ?, but Store.init only creates thenotestable — every call raises sqlite3.OperationalError (no such table: users), so the upcoming feed view will crash the moment it calls this.Fix: Create the
userstable in Store.init (with anid+display_namecolumn) before querying it, or redefine the author lookup against a table that actually exists.🪙 ~1640 tok (41% · attributed output)
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.