Damoose: NIP-07 Safari extension and DIP-05 iOS signer - #3506
Damoose: NIP-07 Safari extension and DIP-05 iOS signer#3506alltheseas wants to merge 17 commits into
Conversation
Implement nostrsigner:// URL scheme handler allowing other iOS nostr apps to use Damus as their signer via URL callbacks. New files: - NostrSignerRequest: Parse incoming nostrsigner:// URLs - NostrSignerResponse: Build callback URLs with results - NostrSignerHandler: Coordinate signing flow - NostrSignerApprovalView/Sheet: User approval UI - SigningPolicyManager: Permission evaluation framework - ContactListPolicy: Protect against mass unfollows - SharedKeychainStorage: Access keypair for signing - DIP-05.md: Protocol specification Features: - Explicit encoding parameter (url/base64url) for content - Re-entrancy prevention (block nostrsigner callbacks) - Operation-specific result encoding (hex/ciphertext/base64) - Policy-based auto-approve/deny with user override 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Implement Safari Web Extension that provides window.nostr for web apps. Architecture: - injected.js: Injects window.nostr into page context - content.js: Bridges page and extension contexts - background.js: Routes to native handler - SafariWebExtensionHandler.swift: Native message handler Current functionality: - getPublicKey: Reads from shared UserDefaults - signEvent: Placeholder (will delegate to DIP-05) - nip04/nip44: Placeholder The extension delegates signing operations to the main Damus app via the DIP-05 nostrsigner:// URL scheme for consistent approval UX. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Safari Web Extensions cannot receive URL callbacks directly, so we use App Group UserDefaults as a communication bridge: 1. Extension receives signEvent, stores request with unique ID 2. Extension returns nostrsigner:// URL for JS to open 3. Damus app signs, stores result keyed by request ID 4. User switches back to Safari, JS polls for result 5. Extension checks shared storage, returns signed event New files: - SignerBridgeStorage: App Group storage for extension<->app IPC Changes: - NostrSignerRequest: Add extensionRequestId for bridge requests - NostrSignerHandler: Store results in bridge for extension requests - SafariWebExtensionHandler: Implement signEvent delegation - JS files: Handle URL opening and result polling 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The main app uses `group.com.damus` for shared UserDefaults, with automatic mirroring via DamusUserDefaults. The Safari extension was incorrectly using `group.com.jb55.damus2`. Now both use `group.com.damus`, so: - Main app writes pubkey to shared UserDefaults on login - Safari extension can read pubkey for getPublicKey - SignerBridgeStorage uses same App Group for IPC 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Restores the original Damoose Safari Web Extension implementation: - In-browser iframe popup for approval UI (no app switching) - Request queue by host in background.js - nostr.js providing window.nostr API - Native handler with request decoding Fixed Info.plist to include required bundle keys. Added Damoose target to Xcode project. Based on commits fe3ec36, 1bbf0f8, 99bb078 by jb55. Closes: damus-624 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…t delegation - Add App Group and Keychain entitlements to Damoose.entitlements - Update SafariWebExtensionHandler to read pubkey from shared UserDefaults - Implement signEvent delegation: stores request, JS opens nostrsigner:// URL - Add checkResult for polling signed event results - Fix bug where response was never returned to extension context - Use consistent key prefixes matching SignerBridgeStorage Closes damus-ydw, damus-399, damus-gtf 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add docstrings to all types and functions per code standards - Remove unused getStoredPrivateKey() function - Remove unused encoder variable in handleSignEvent() 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Uncomment setup_nip07() and setup_iframe() to enable: - window.nostr injection into pages - Half-screen iframe popup for request approval Remove erroneous setup_highlighter() call (already runs in background.js). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add secp256k1 to Damoose target and implement direct event signing: - Link secp256k1 Swift package to extension target - Add crypto helpers: hexDecode, hexEncode, sha256, randomBytes - Add eventCommitment and calculateEventId for nostr event ID - Add signEventId for schnorr signing - Update handleSignEvent to sign directly without app switching - Remove checkResult polling (no longer needed) No more app switching for signing - extension handles it inline. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ion setup UI Wire popup approval UI to permission storage for remembered permissions. Add Safari Extension settings view in Damus app settings. - Add permission storage (isPermissionApproved, savePermission) in native handler - Add checkPermission request type for JS to query saved permissions - Update popup.js with "Remember this permission" checkbox - Update content.js to check permissions before showing popup - Update background.js to handle checkPermission and pass remember/origin - Add SafariExtensionSettingsView with setup instructions - Add route and navigation link in ConfigView 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add missing opening parenthesis in browser.contextMenus.create() call. This syntax error prevented the background script from loading entirely. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Validate message.source to prevent forged approve/deny messages: - Popup messages (approve/deny/popup_initialized) must come from iframe - NIP-07 messages (signEvent/getPubKey) must come from window context Without this check, a malicious page could forge approval messages and trigger signing without user consent. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
content.js sends { payload: host_state.requests } but popup.js was
accessing payload.requests. Now correctly uses payload directly.
This fix ensures queued requests render in the popup on initialization.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
completeApproval() returns nil for Safari extension requests (result stored in bridge storage, no callback URL needed). Previously this was incorrectly treated as an error showing "Failed to generate response". Now correctly dismisses the sheet for extension requests, allowing users to switch back to Safari manually. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace placeholder 'application.id' with correct 'damoose' identifier to match the Safari extension handler configuration. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughIntroduces NIP-55 and NIP-07 compatible Nostr signing infrastructure, including two Safari web extensions (Damoose and DamusNostrSigner), a signing policy evaluation framework with per-client permission management, cross-extension communication bridges, and associated UI for user approval workflows. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Web as Web App
participant Ext as Safari<br/>Extension
participant Handler as Native<br/>Handler
participant App as Damus App
participant Storage as Keychain &<br/>Defaults
Web->>Ext: window.nostr.signEvent(event)
activate Ext
Ext->>Handler: browser.runtime.sendNativeMessage<br/>(signEvent + origin)
deactivate Ext
activate Handler
Handler->>Storage: getPublicKey() &<br/>getPrivateKey()
activate Storage
Storage-->>Handler: pubkey, privkey
deactivate Storage
Handler->>Handler: Verify permission<br/>for origin + kind
alt Permission exists & approved
Handler->>Handler: Sign event<br/>(Schnorr)
Handler-->>Ext: { signature, event }
else Permission denied
Handler-->>Ext: { error: rejected }
else First-time request
Handler->>App: (Decision: requires approval)
end
deactivate Handler
activate Ext
alt Auto-sign
Ext->>Web: Promise resolves<br/>with signature
else Requires approval
Ext->>Ext: Show popup iframe
activate App
User->>App: Review & approve
App->>Handler: Update permissions
Handler->>Storage: Save approval
App-->>Ext: Permission granted
deactivate App
Ext->>Handler: Retry signEvent
Handler->>Handler: Sign event
Handler-->>Ext: { signature, event }
Ext->>Web: Promise resolves
end
deactivate Ext
sequenceDiagram
actor User
participant App1 as Requesting<br/>App
participant Damus as Damus App
participant Handler as Nostr Signer<br/>Handler
participant Manager as Signing Policy<br/>Manager
participant UI as Approval<br/>Sheet
participant Storage as Shared<br/>Storage
App1->>Damus: Open nostrsigner://<br/>(signEvent + callback)
activate Damus
Damus->>Handler: parse & handle(request)
activate Handler
Handler->>Manager: evaluate(event, client)
activate Manager
Manager->>Storage: load permissions<br/>(client)
activate Storage
Storage-->>Manager: ClientPermissions
deactivate Storage
Manager->>Manager: Check trust level<br/>& kind approval
alt Auto-approve
Manager-->>Handler: approve
else Unknown/not approved
Manager-->>Handler: requiresApproval<br/>(context, risks)
else Blocked
Manager-->>Handler: deny
end
deactivate Manager
alt Auto-approve
Handler->>Handler: Sign event<br/>(key from keychain)
Handler->>Damus: Return callback URL<br/>(sig + event)
Damus->>App1: Open callback URL
else Requires approval
Handler->>UI: Present approval sheet
activate UI
User->>UI: Review request<br/>& risks
User->>UI: Approve/Deny +<br/>Remember?
deactivate UI
Handler->>Manager: updatePermissions<br/>(if remember)
activate Manager
Manager->>Storage: Save decision
deactivate Manager
Handler->>Handler: Sign event
Handler->>Damus: Return callback URL
Damus->>App1: Open callback URL
else Denied
Handler->>Damus: Error callback
Damus->>App1: Open error URL
end
deactivate Handler
deactivate Damus
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Comment |
alltheseas
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🤖 Fix all issues with AI Agents
In @.gitattributes:
- Around line 1-3: The .gitattributes entry adds a custom merge driver
(merge=beads) but the PR lacks any git configuration or documentation for that
driver and also mixes unrelated Beads infra changes into a feature PR; add
explicit setup docs showing the required git config (e.g., the
merge.beads.driver command and any helper script usage) in .beads/README.md or a
CONTRIBUTING/AGENTS.md section, and split the Beads-specific files
(.gitattributes, .beads/, .beads/README.md, .beads/.gitignore, AGENTS.md bd sync
workflow) into a separate infrastructure PR so the Damoose NIP-07/DIP-05 signing
solution PR only contains signing-related changes.
In @Damoose/Info.plist:
- Line 29: Update the copyright string in Info.plist by changing the year from
"Copyright 2024 Damus. All rights reserved." to an appropriate current range or
year (e.g., "Copyright 2026 Damus. All rights reserved." or "Copyright 2024-2026
Damus. All rights reserved."), ensuring the exact quoted string value in the
<string> element is replaced accordingly.
In @Damoose/Resources/background.js:
- Around line 25-50: The context menu click handler is wired incorrectly: remove
the second argument from browser.contextMenus.create in setup_highlighter and
instead register the click handler with
browser.contextMenus.onClicked.addListener(handle_menu_item_click); update
handle_menu_item_click to accept (info, tab) and destructure selectionText,
srcUrl, mediaType, linkUrl, pageUrl directly from info (not event.userInfo),
compute value = (mediaType === 'image' ? srcUrl : (linkUrl || selectionText)),
and then call browser.runtime.sendNativeMessage("damoose", { kind: "highlight",
payload: { mediaType, value, selectionText, pageUrl } }).
- Around line 3-10: The const declaration nativePayload inside the switch
'approve' case has function-scoped switch fall-through issues; wrap the case
body in its own block (e.g., case 'approve': { ... } ) so nativePayload is
scoped locally and then call browser.runtime.sendNativeMessage("damoose",
nativePayload) inside that block; ensure you close the block before the
break/return to prevent leaking the binding to other cases.
In @Damoose/Resources/content.js:
- Around line 64-68: Remove the debug keyboard trigger by deleting or disabling
the keydown listener that checks for event.key === 'o' and calls toggle_popup();
specifically remove the document.addEventListener('keydown', ...) block (the
handler referencing the 'o' key and toggle_popup) or gate it behind a
development-only flag so that in production the keypress cannot open the signing
popup.
In @Damoose/Resources/manifest.json:
- Around line 17-21: Remove the invalid "run_at" field from the "background"
entry in the manifest (the object containing "scripts": ["background.js"] and
"type": "module"); background/service worker entries in Manifest V3 do not
support "run_at", so delete that property and leave the "background" object with
only valid keys (e.g., "scripts" and "type") to conform to MV3.
- Around line 28-39: The manifest currently exposes nostr.js, popup.js, and
popup.html to "<all_urls>", so ensure the message origin validation in
content.js (the message handler around lines 89-106) remains intact: preserve
the iframe-origin check for popup messages, the window-context check for NIP-07
messages, and the default reject path for all other messages; add
unit/integration tests or inline comments around the message handler to prevent
accidental removal during refactors and keep the iframe sandbox attributes
("allow-scripts allow-same-origin") and permission gating unchanged.
In @Damoose/Resources/nostr.js:
- Around line 23-27: Fix the typo in the comment above the enable() method:
change "comatibility" to "compatibility" in the comment that starts "This is
here for Alby comatibility." so the comment correctly reads "This is here for
Alby compatibility. This is not part of the NIP-07 standard." and leave the
async enable() { return { enabled: true }; } code unchanged.
In @Damoose/SafariWebExtensionHandler.swift:
- Around line 177-182: The randomBytes(count:) function currently ignores
SecRandomCopyBytes' return value; update it to check the result of
SecRandomCopyBytes(kSecRandomDefault, count, &bytes) and handle non-success
(non-errSecSuccess) cases instead of returning zeroed bytes — either throw an
error (change signature to throws and propagate a descriptive error), or abort
with a clear precondition/fatalError message; ensure the calling code handles
the new error path. Use the symbol randomBytes(count:) and the
SecRandomCopyBytes call to locate and modify the implementation.
In @damus.xcodeproj/project.pbxproj:
- Around line 263-264: The PBXBuildFile entries for the secp256k1 items are
merged into one malformed line (identifiers 4C649881286E0EE300EAE2B3 and
A14FAA9F3D34738FBCE6392D with productRefs 6BAC5FED0B6276CB910C650F and
4C649880286E0EE300EAE2B3), which breaks Xcode parsing; fix by splitting this
into two distinct PBXBuildFile blocks so each has its own "isa = PBXBuildFile;"
and its correct "productRef = <...>;" and terminating semicolons/newlines (one
block for 4C649881286E0EE300EAE2B3 referencing productRef
4C649880286E0EE300EAE2B3, and a separate block for A14FAA9F3D34738FBCE6392D
referencing productRef 6BAC5FED0B6276CB910C650F), restoring proper PBXProject
syntax.
- Line 1522: Remove the orphaned PBXBuildFile entry named
D73E5EFE2C6A97F4007EB227: delete the entire PBXBuildFile block that has isa =
PBXBuildFile and no fileRef, and also remove any reference to
D73E5EFE2C6A97F4007EB227 from the PBXSourcesBuildPhase "files" array so the
build phase no longer points to the missing BuildFile; ensure no other sections
reference that identifier after removal.
- Around line 3070-3077: The DamusNostrSigner extension is missing the images
PBXGroup and the actual icon assets referenced by manifest.json; create
DamusNostrSigner/Resources/images/ and add the files images/icon-48.png,
images/icon-96.png, images/icon-128.png (matching the manifest.json names), then
update the PBXProject so the PBXGroup with name = images (isa = PBXGroup)
includes those file references as children and ensure they are added to the
extension target’s Copy Bundle Resources; also update the PBXGroup for name =
_locales to include a path attribute pointing to the on-disk _locales directory
so Xcode can resolve the locale subfolders and their file references.
In @damus/Damoose/NIP07/SignerBridgeStorage.swift:
- Around line 175-197: The cleanup() routine is never called so stale
UserDefaults entries accumulate; add invocations of
SignerBridgeStorage.cleanup() at key interaction points such as at the start of
storeRequest(...), at the start of getResult(...), and once during app
launch/initialization (e.g., when the SignerBridgeStorage singleton or host app
initializes) so expired entries are removed before storing or reading data;
ensure calls are lightweight and keep the existing implementation unchanged.
- Around line 60-102: SafariWebExtensionHandler.storeRequest() duplicates the
logic of SignerBridgeStorage.storeRequest(); replace the duplicated
implementation in SafariWebExtensionHandler.storeRequest() with a call to
SignerBridgeStorage.storeRequest(eventJson:origin:) and propagate its returned
requestId (or handle nil) instead of reimplementing UUID generation,
UserDefaults access, and synchronization to ensure single source of truth and
avoid duplicated code.
In @damus/Damoose/NIP55/NostrSignerApprovalView.swift:
- Around line 174-176: The ForEach is using non-unique id: \.description which
can duplicate for multiple SigningRisk cases; change it to use a truly unique
identifier such as an index or a unique property on SigningRisk: replace
ForEach(context.risks.sorted { $0.severity > $1.severity }, id: \.description)
with a variant that uses indices (e.g. enumerated/indexed iteration) or a unique
id field on SigningRisk (e.g. id or uuid) and keep the body calling
riskRow(risk) unchanged so SwiftUI has stable, unique IDs for each row.
In @damus/Damoose/NIP55/NostrSignerHandler.swift:
- Around line 306-315: waitForApproval stores approvalContinuation and can leak
if the approval sheet is dismissed without calling approveRequest/denyRequest;
add a cancelApproval() method that calls clearPendingRequest(), resumes
approvalContinuation returning false, and nils it (mirror logic used in
approveRequest/denyRequest), then invoke cancelApproval from the sheet's
onDismiss handler so the continuation is always resumed and no async task is
left hanging.
In @DamusNostrSigner/SafariWebExtensionHandler.swift:
- Around line 203-229: The problem is that buildSignerUrl uses .urlPathAllowed
to percent-encode eventJson (encodedEvent) which doesn't escape characters like
&, +, =, ? that can break the constructed nostrsigner URL; change the encoding
to use a safer character set for a query/value context (e.g., start from
CharacterSet.urlQueryAllowed and remove characters such as "&", "+", "=", "?",
"/" and ":" that must be percent-escaped in this position) so
addingPercentEncoding is called with that custom CharacterSet for encodedEvent
(leave encodedCallback as urlQueryAllowed or use the same safe set), then
rebuild the URL using the newly encodedEvent and encodedCallback.
🟡 Minor comments (10)
Damoose/Info.plist-29-29 (1)
29-29: Update copyright year.The copyright year is set to 2024, but since this is new code being added in 2026, consider updating it to "Copyright 2026 Damus. All rights reserved." or "Copyright 2024-2026 Damus. All rights reserved."
AGENTS.md-48-67 (1)
48-67: Document setup for thebdtool in the mandatory workflow.The workflow at lines 48-67 introduces
bd syncas a mandatory step, but no documentation exists explaining what Beads is or how to install thebdtool. This violates the guideline to update documentation when workflows change. Add setup instructions to README.md or docs/ so contributors can complete the workflow without encountering failures.Damoose/Resources/manifest.json-28-39 (1)
28-39: Web accessible resources exposure requires robust origin validation.The broad exposure of
nostr.js,popup.js, andpopup.htmlto"<all_urls>"is necessary for this NIP-07 signer extension to function on any Nostr-compatible website, but it does increase the extension's attack surface.Good news: message origin validation is already properly implemented in
content.js(lines 89-106):
- Popup messages validated to come only from the iframe
- NIP-07 messages validated to come from the page's window context
- All other messages rejected
Ensure this validation logic remains robust and is maintained through future refactors. The iframe sandbox restrictions (
allow-scripts allow-same-origin) and permission system also provide good defense-in-depth.Damoose/Resources/background.js-3-10 (1)
3-10: Wrapconstdeclaration in a block to fix switch scope issue.As flagged by static analysis, the
const nativePayloaddeclaration inside the switch case can be erroneously accessed by other clauses. Wrap it in a block.🔎 Proposed fix
switch (message.kind) { - case 'approve': + case 'approve': { // Include remember flag and origin for permission storage const nativePayload = { ...message.payload, remember: message.remember ?? false, origin: message.origin ?? "" } return browser.runtime.sendNativeMessage("damoose", nativePayload) + } case 'deny':Damoose/Resources/content.js-64-68 (1)
64-68: Remove debug keyboard trigger before release.The 'o' key toggle appears to be a development convenience. Users shouldn't be able to toggle the signing popup via keyboard as it could be confusing or exploited by malicious pages simulating key events.
🔎 Proposed fix
- // Example trigger - document.addEventListener('keydown', function(event) { - if (event.key === 'o') { // Press 'o' to show the iframe popup - toggle_popup(); - } - });damus/Damoose/NIP55/NostrSignerApprovalView.swift-174-176 (1)
174-176: Use a unique identifier forForEachinstead ofid: \.description.The
descriptionproperty is not guaranteed to be unique across differentSigningRiskinstances. Cases like.unknownClient,.contactListEmpty,.deletionEvent, and.encryptedContentalways return the same static string, meaning multiple risks incontext.riskscan share identical descriptions. SwiftUI'sForEachwith non-unique identifiers can cause undefined behavior, including incorrect state management and rendering issues. Use a unique ID mechanism—such as an index-based approach or a dedicated unique identifier field inSigningRisk.Damoose/Resources/nostr.js-23-27 (1)
23-27: Typo in comment."comatibility" should be "compatibility".
damus/Damoose/NIP55/NostrSignerHandler.swift-306-315 (1)
306-315: Potential continuation leak if approval sheet is dismissed without decision.The
waitForApprovalfunction stores a continuation that expectsapproveRequestordenyRequestto be called. If the user dismisses the approval sheet via swipe-down or other means without explicitly approving/denying, the continuation may never resume, leaving the async task hanging.Consider adding cleanup logic when the sheet is dismissed:
🔎 Suggested approach
Add a
cancelApproval()method that resumes the continuation withfalse:func cancelApproval() { clearPendingRequest() approvalContinuation?.resume(returning: false) approvalContinuation = nil }Then call this from the sheet's
onDismisshandler.DamusNostrSigner/SafariWebExtensionHandler.swift-203-229 (1)
203-229: URL encoding may fail for certain characters in event JSON.Using
.urlPathAllowedfor encoding the event JSON (line 205-207) may not properly encode all characters that can appear in JSON (e.g.,+,&,=,?). Consider using.urlQueryAllowedwith additional characters removed, or a custom character set.🔎 Proposed fix for safer URL encoding
private func buildSignerUrl(eventJson: String, requestId: String, origin: String) -> String? { // URL-encode the event JSON + var allowedChars = CharacterSet.urlPathAllowed + allowedChars.remove(charactersIn: "+&=?#") guard let encodedEvent = eventJson.addingPercentEncoding( - withAllowedCharacters: .urlPathAllowed + withAllowedCharacters: allowedChars ) else { return nil }DamusNostrSigner/SafariWebExtensionHandler.swift-166-184 (1)
166-184: Remove code duplication betweenSafariWebExtensionHandlerandSignerBridgeStorage, or document why duplication is necessary.The
storeRequestandgetResultmethods inSafariWebExtensionHandlerduplicate logic fromSignerBridgeStoragewith identical key prefixes (signer_request_,signer_result_). If the extension target cannot accessSignerBridgeStoragedue to app extension sandboxing limitations, this duplication is justified—but should be documented. Otherwise, consider refactoring to share the implementation.
🧹 Nitpick comments (24)
damus/Damoose/DIP-05.md (5)
21-23: Add language identifier to fenced code block.This code block should specify a language (e.g.,
url,text) for proper syntax highlighting and markdown compliance.-``` +```url nostrsigner:<content>?type=<method>&callbackUrl=<url>&compressionType=<type>&returnType=<format>&pubkey=<hex> -``` +```
35-35: Fix formatting issue in table cell.Line 35 contains
**Yes***which renders with mismatched asterisks. This should be either**Yes***(bold with asterisk) or reformatted clearly.-| `callbackUrl` | **Yes*** | URL-encoded callback | Where to send result (e.g., `primal://nostrsigner`) | +| `callbackUrl` | **Yes\*** | URL-encoded callback | Where to send result (e.g., `primal://nostrsigner`) |
57-63: Add language identifier to fenced code block.This code block should specify a language for consistency and markdown compliance.
-``` +```text # callbackUrl has no params: primal://nostrsigner → primal://nostrsigner?result=<sig> # callbackUrl already has params: primal://callback?session=123 → primal://callback?session=123&result=<sig> -``` +```
104-116: Add language identifier to fenced code block.This code block should specify a language for markdown compliance.
-``` +```text 1. Primal wants to post a note 2. Primal opens: nostrsigner:%7B%22kind%22%3A1%2C%22content%22%3A%22Hello%22%7D?type=sign_event&callbackUrl=primal%3A%2F%2Fnostrsigner&returnType=event 3. iOS switches to Damus 4. Damus parses request, shows approval UI 5. User approves 6. Damus signs event, opens: primal://nostrsigner?result=abc123...&event=%7B%22id%22%3A%22...%22%2C%22sig%22%3A%22abc123...%22%7D 7. iOS switches back to Primal with signed event -``` +```
118-131: Consider adding timeout and edge-case guidance to security section.The security section is strong but could benefit from additional guidance on:
- Timeout behavior: How long should the signer wait for user approval before timing out?
- Edge cases: What happens if
callbackUrlparsing fails or the app is uninstalled before the callback completes?These are optional enhancements for completeness.
damus.xcodeproj/project.pbxproj (2)
8452-8490: Consider adding-DEXTENSIONSwift flag for consistency with other extensions.Other extension targets (ShareExtension, HighlighterActionExtension, DamusNotificationService) include
OTHER_SWIFT_FLAGS = "-DEXTENSION"to enable conditional compilation. The newDamooseandDamusNostrSignertargets don't have this flag.If any shared Swift files use
#if EXTENSIONpreprocessor checks, these extensions may behave unexpectedly.🔎 Proposed fix for Damoose Debug configuration
5048BF14FD7EB52DC4C5A5C5 /* Debug */ = { isa = XCBuildConfiguration; buildSettings = { CLANG_ENABLE_OBJC_WEAK = NO; CODE_SIGN_ENTITLEMENTS = Damoose/Damoose.entitlements; CURRENT_PROJECT_VERSION = 1; GENERATE_INFOPLIST_FILE = NO; INFOPLIST_FILE = Damoose/Info.plist; IPHONEOS_DEPLOYMENT_TARGET = 16.0; MARKETING_VERSION = 1.0; + OTHER_SWIFT_FLAGS = "-DEXTENSION"; PRODUCT_BUNDLE_IDENTIFIER = com.jb55.damus2.Damoose; PRODUCT_NAME = Damoose; SDKROOT = iphoneos; SWIFT_VERSION = 5.0; }; name = Debug; };Apply similar changes to Release configurations for both Damoose and DamusNostrSigner.
2032-2042: Consider consolidating extension embedding into a single build phase.There are now two separate copy files phases for embedding extensions:
- "Embed Foundation Extensions" (contains Damoose, ShareExtension, etc.)
- "Embed App Extensions" (contains only DamusNostrSigner)
Both use
dstSubfolderSpec = 13(PlugIns folder). While this works, consolidating into a single phase improves maintainability.damus/Features/Settings/Views/SafariExtensionSettingsView.swift (2)
56-65: Consider adding a docstring for the private helper.While this is a private helper function, adding a brief docstring would improve maintainability and align with the coding guidelines requiring docstring coverage for added code.
🔎 Suggested docstring
+ /// Renders a numbered instruction row with consistent formatting. + /// - Parameters: + /// - number: The step number to display + /// - text: The instruction text + /// - Returns: A view displaying the formatted instruction private func instructionRow(number: Int, text: String) -> some View {
67-70: Consider adding a docstring.Adding a brief docstring for this helper would improve code documentation.
🔎 Suggested docstring
+ /// Opens the system Settings app. private func openSafariSettings() {Damoose/Resources/popup.css (1)
12-14: Empty dark mode media query - intentional or TODO?The dark mode media query is defined but empty. If dark mode styling is not needed (relying on
color-scheme: light darkfor system defaults), consider removing the empty rule. Otherwise, add appropriate dark mode styles.🔎 Option to remove if not needed
-@media (prefers-color-scheme: dark) { - /* Dark Mode styles go here. */ -}Damoose/Resources/popup.js (1)
5-18: Consider adding origin validation for defense in depth.While the parent content.js validates message sources before forwarding to this popup, validating the message origin here as well would provide defense in depth against potential future changes that might bypass the parent's validation.
Damoose/Resources/content.js (1)
2-7: Remove unused state properties.
reqidsandapprovedare initialized but never used. Theget_requesthelper (lines 20-22) is also unused.🔎 Proposed cleanup
let host_state = { requests: {}, - reqids: 0, - approved: {}, iframe: null, // Reference to popup iframe for origin validation }Also remove the unused
get_requestfunction at lines 20-22.damus/Damoose/NIP55/NostrSignerApprovalSheet.swift (1)
146-157: RedundantDispatchQueue.main.asyncinside MainActor context.
openCallbackis called fromhandleApprovalwhich is@MainActor. TheDispatchQueue.main.asyncwrapper is unnecessary.🔎 Proposed simplification
/// Opens the callback URL to return result to requesting app. private func openCallback(_ url: URL) { - // Use UIApplication to open the URL - // This will switch to the requesting app - DispatchQueue.main.async { - UIApplication.shared.open(url, options: [:]) { success in - if !success { - print("NostrSigner: Failed to open callback URL: \(url)") - } + UIApplication.shared.open(url, options: [:]) { success in + if !success { + print("NostrSigner: Failed to open callback URL: \(url)") } } }DamusNostrSigner/Resources/background.js (1)
74-102: Consider distinguishing transient errors from permanent failures.The catch block at lines 95-98 logs and continues on any error, which is appropriate for transient failures. However, if the native messaging connection is permanently broken (e.g., extension unloaded), this will silently poll until timeout. Consider tracking consecutive errors and failing fast after a threshold.
Proposed enhancement
async function pollForResult(requestId, maxAttempts = 300, intervalMs = 1000) { + let consecutiveErrors = 0; + const maxConsecutiveErrors = 5; + for (let i = 0; i < maxAttempts; i++) { await sleep(intervalMs); try { const response = await browser.runtime.sendNativeMessage('damoose', { method: 'checkResult', params: { requestId } }); + consecutiveErrors = 0; // Reset on success + if (response.pending) { continue; } if (response.error) { throw new Error(response.error); } return response.result; } catch (e) { + consecutiveErrors++; console.error('Poll error:', e); + if (consecutiveErrors >= maxConsecutiveErrors) { + throw new Error('Native messaging connection lost'); + } } } throw new Error('Signing timed out - please try again'); }DamusNostrSigner/Resources/content.js (1)
44-50: Variableeshadows outer scope.The catch variable
eat line 48 shadows the event parameterefrom the outer listener. While this works due to JS scoping rules, it reduces clarity.Proposed fix
- window.addEventListener('DAMUS_START_POLL', (e) => { - const { id, requestId } = e.detail; + window.addEventListener('DAMUS_START_POLL', (event) => { + const { id, requestId } = event.detail; pendingPolls.set(requestId, id); // Store in sessionStorage so poll survives page navigation try { const stored = JSON.parse(sessionStorage.getItem('damus_pending_polls') || '{}'); stored[requestId] = id; sessionStorage.setItem('damus_pending_polls', JSON.stringify(stored)); - } catch (e) { + } catch (err) { // sessionStorage may not be available } });DamusNostrSigner/Resources/injected.js (1)
59-85: Guard against missingdocument.bodyon early page load.If
handleOpenUrlActionis called before the DOM is fully ready,document.bodymay be null, causing the notification append to fail silently or throw.Proposed fix
function handleOpenUrlAction(id, actionData) { const { url, requestId: extRequestId } = actionData; // Show user feedback before switching apps const notification = document.createElement('div'); notification.style.cssText = ` position: fixed; top: 20px; right: 20px; background: #1a1a2e; color: white; padding: 16px 24px; border-radius: 12px; z-index: 999999; font-family: -apple-system, BlinkMacSystemFont, sans-serif; box-shadow: 0 4px 12px rgba(0,0,0,0.3); `; notification.textContent = 'Opening Damus for signing...'; - document.body.appendChild(notification); + (document.body || document.documentElement).appendChild(notification); // Open the URL to switch to Damus window.location.href = url;damus/Damoose/NIP55/NostrSignerRequest.swift (1)
147-151: Remove redundant optional initialization.Static analysis correctly identifies that initializing an optional to
nilis redundant.Proposed fix
// Target pubkey for encrypt/decrypt operations - var targetPubkey: Pubkey? = nil + var targetPubkey: Pubkey? if let pubkeyHex = queryItems.first(where: { $0.name == "pubkey" })?.value { targetPubkey = hex_decode_pubkey(pubkeyHex) }Damoose/Resources/nostr.js (1)
29-35: Promises may never resolve, causing memory leaks.The
broadcastfunction stores resolve callbacks inthis.requestsbut never rejects or times out. If the content script/extension fails to respond, these promises hang forever and the entries accumulate in therequestsmap.Consider adding a timeout mechanism:
🔎 Proposed fix with timeout
broadcast(kind, payload) { let reqId = Math.random().toString(); return new Promise((resolve, _reject) => { this.requests[reqId] = resolve; window.postMessage({ kind, reqId, payload }, '*'); + + // Cleanup after 60 seconds to prevent memory leaks + setTimeout(() => { + if (this.requests[reqId]) { + delete this.requests[reqId]; + _reject(new Error('Request timed out')); + } + }, 60000); }); },Damoose/SafariWebExtensionHandler.swift (2)
213-214: Auxiliary random should be 32 bytes for schnorr signatures.The
secp256k1schnorr signature typically expects 32 bytes of auxiliary randomness, but 64 bytes are generated here. While this may work (library may truncate), it's wasteful and potentially confusing.🔎 Proposed fix
- var auxRand = randomBytes(count: 64) + var auxRand = randomBytes(count: 32)
230-280: Consider consolidating message parsing with more robust validation.The
decode_damoose_requestfunction handles multiple message types but returnsnilsilently for partially valid messages (e.g.,signEventwith missingcontent). Consider logging which specific field failed for debugging purposes.damus/Damoose/SigningPolicy.swift (1)
115-127:Equatableimplementation forPolicyDecisionignores most context.The custom
Equatablefor.requireApprovalonly comparessummary, ignoringclient,event, andrisks. This could cause incorrect equality checks in tests or other logic.🔎 Proposed fix for more complete equality
case (.requireApproval(let c1), .requireApproval(let c2)): - return c1.summary == c2.summary + return c1.summary == c2.summary && + c1.client == c2.client && + c1.event == c2.event && + c1.risks == c2.risksNote: This requires
ApprovalContextto conform toEquatable.damus/Damoose/NIP55/NostrSignerHandler.swift (2)
78-102: TODO: Implement encryption/decryption methods.The TODO at lines 95-96 for NIP-04/NIP-44 encryption is noted. Consider tracking this in an issue.
Would you like me to open a GitHub issue to track the implementation of encryption/decryption methods?
169-224: Unusedprivkeyvariable after guard check.At line 175,
privkeyis extracted fromkeypairbut never used - the signing is done viaNostrEventinitializer which useskeypairdirectly. The guard is effectively just an existence check.🔎 Proposed fix to clarify intent
// Need private key to sign - guard let privkey = keypair.privkey else { + guard keypair.privkey != nil else { return handleError( request: request, message: "No private key available (read-only mode)", rejected: false ) }damus/Damoose/SigningPolicyManager.swift (1)
115-128: Kind policyapprovedecision is ignored.When a kind-specific policy returns
.approve, the code falls through to checkpermissions.isKindApprovedagain. This means a kind policy cannot grant approval for unapproved kinds. If this is intentional (kind policy can only deny/require-approval), document this behavior.🔎 Suggested fix if approve should be respected
// Run kind-specific policy if available if let kindPolicy = kindPolicies[event.kind] { let policyDecision = kindPolicy.evaluate(event: event, client: client) + // If kind policy approves, respect that + if case .approve = policyDecision { + recordRequest(for: client) + return policyDecision + } + // If kind policy denies, respect that if case .deny = policyDecision { return policyDecision }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
Damoose/Resources/images/icon-128.pngis excluded by!**/*.pngDamoose/Resources/images/icon-256.pngis excluded by!**/*.pngDamoose/Resources/images/icon-48.pngis excluded by!**/*.pngDamoose/Resources/images/icon-512.pngis excluded by!**/*.pngDamoose/Resources/images/icon-64.pngis excluded by!**/*.pngDamoose/Resources/images/icon-96.pngis excluded by!**/*.pngDamoose/Resources/images/toolbar-icon.svgis excluded by!**/*.svg
📒 Files selected for processing (47)
.beads/.gitignore.beads/README.md.beads/config.yaml.beads/interactions.jsonl.beads/issues.jsonl.beads/metadata.json.gitattributesAGENTS.mdDamoose/Damoose.entitlementsDamoose/Info.plistDamoose/Resources/_locales/en/messages.jsonDamoose/Resources/background.jsDamoose/Resources/content.jsDamoose/Resources/manifest.jsonDamoose/Resources/nostr.jsDamoose/Resources/popup.cssDamoose/Resources/popup.htmlDamoose/Resources/popup.jsDamoose/SafariWebExtensionHandler.swiftDamusNostrSigner/DamusNostrSigner.entitlementsDamusNostrSigner/Info.plistDamusNostrSigner/Resources/background.jsDamusNostrSigner/Resources/content.jsDamusNostrSigner/Resources/injected.jsDamusNostrSigner/Resources/manifest.jsonDamusNostrSigner/SafariWebExtensionHandler.swiftdamus.xcodeproj/project.pbxprojdamus.xcodeproj/xcshareddata/xcschemes/Damoose.xcschemedamus.xcodeproj/xcshareddata/xcschemes/DamusNostrSigner.xcschemedamus/ContentView.swiftdamus/Damoose/ClientPermissions.swiftdamus/Damoose/DIP-05.mddamus/Damoose/NIP07/SignerBridgeStorage.swiftdamus/Damoose/NIP55/NostrSignerApprovalSheet.swiftdamus/Damoose/NIP55/NostrSignerApprovalView.swiftdamus/Damoose/NIP55/NostrSignerHandler.swiftdamus/Damoose/NIP55/NostrSignerRequest.swiftdamus/Damoose/NIP55/NostrSignerResponse.swiftdamus/Damoose/Policies/ContactListPolicy.swiftdamus/Damoose/SharedKeychainStorage.swiftdamus/Damoose/SigningPolicy.swiftdamus/Damoose/SigningPolicyManager.swiftdamus/Features/Settings/Views/ConfigView.swiftdamus/Features/Settings/Views/SafariExtensionSettingsView.swiftdamus/Info.plistdamus/Shared/Utilities/Router.swiftdamus/Shared/Utilities/URLHandler.swift
🧰 Additional context used
📓 Path-based instructions (1)
**/*.swift
📄 CodeRabbit inference engine (AGENTS.md)
**/*.swift: Maximize usage of nostrdb facilities (Ndb, NdbNote, iterators) whenever possible for persistence and queries in the Damus iOS app
Favor Swift-first solutions that lean on nostrdb types (Ndb, NdbNote, iterators) before introducing new storage mechanisms
Ensure docstring coverage for any code added, or modified
Ensure nevernesting: favor early returns and guard clauses over deeply nested conditionals; simplify control flow by exiting early instead of wrapping logic in multiple layers of if statements
Files:
damus/Features/Settings/Views/ConfigView.swiftdamus/Shared/Utilities/Router.swiftdamus/Features/Settings/Views/SafariExtensionSettingsView.swiftdamus/Damoose/NIP07/SignerBridgeStorage.swiftDamusNostrSigner/SafariWebExtensionHandler.swiftdamus/Damoose/SharedKeychainStorage.swiftdamus/Damoose/NIP55/NostrSignerResponse.swiftdamus/Damoose/ClientPermissions.swiftdamus/Damoose/SigningPolicy.swiftDamoose/SafariWebExtensionHandler.swiftdamus/Damoose/NIP55/NostrSignerHandler.swiftdamus/Damoose/SigningPolicyManager.swiftdamus/Damoose/Policies/ContactListPolicy.swiftdamus/ContentView.swiftdamus/Damoose/NIP55/NostrSignerApprovalSheet.swiftdamus/Shared/Utilities/URLHandler.swiftdamus/Damoose/NIP55/NostrSignerRequest.swiftdamus/Damoose/NIP55/NostrSignerApprovalView.swift
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: damus-io/damus PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-06T01:28:30.381Z
Learning: Review and follow pull_request_template.md when creating PRs for iOS Damus
📚 Learning: 2026-01-06T01:28:30.381Z
Learnt from: CR
Repo: damus-io/damus PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-06T01:28:30.381Z
Learning: Ensure new targets or resources integrate cleanly with the damus.xcodeproj main scheme
Applied to files:
DamusNostrSigner/Info.plistDamoose/Damoose.entitlementsDamoose/Info.plistdamus.xcodeproj/project.pbxprojdamus.xcodeproj/xcshareddata/xcschemes/DamusNostrSigner.xcschemedamus.xcodeproj/xcshareddata/xcschemes/Damoose.xcscheme
📚 Learning: 2026-01-06T01:28:30.381Z
Learnt from: CR
Repo: damus-io/damus PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-06T01:28:30.381Z
Learning: Update documentation when workflows change, especially this file, README.md, or developer notes
Applied to files:
AGENTS.md
📚 Learning: 2026-01-06T01:28:30.381Z
Learnt from: CR
Repo: damus-io/damus PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-06T01:28:30.381Z
Learning: Review and follow pull_request_template.md when creating PRs for iOS Damus
Applied to files:
AGENTS.mddamus/Damoose/DIP-05.mddamus.xcodeproj/project.pbxprojdamus.xcodeproj/xcshareddata/xcschemes/DamusNostrSigner.xcscheme
📚 Learning: 2026-01-06T01:28:30.381Z
Learnt from: CR
Repo: damus-io/damus PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-06T01:28:30.381Z
Learning: Applies to **/*.swift : Maximize usage of nostrdb facilities (Ndb, NdbNote, iterators) whenever possible for persistence and queries in the Damus iOS app
Applied to files:
damus/Damoose/NIP07/SignerBridgeStorage.swiftdamus.xcodeproj/project.pbxproj
📚 Learning: 2026-01-06T01:28:30.381Z
Learnt from: CR
Repo: damus-io/damus PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-06T01:28:30.381Z
Learning: Applies to **/*.swift : Favor Swift-first solutions that lean on nostrdb types (Ndb, NdbNote, iterators) before introducing new storage mechanisms
Applied to files:
damus/Damoose/NIP07/SignerBridgeStorage.swift
🧬 Code graph analysis (10)
Damoose/Resources/popup.js (1)
Damoose/Resources/nostr.js (2)
message(63-63)reqId(30-30)
damus/Damoose/NIP07/SignerBridgeStorage.swift (1)
DamusNostrSigner/SafariWebExtensionHandler.swift (2)
storeRequest(167-184)getResult(187-198)
DamusNostrSigner/SafariWebExtensionHandler.swift (3)
DamusNostrSigner/Resources/background.js (4)
response(31-34)response(50-50)response(80-83)params(10-10)DamusNostrSigner/Resources/content.js (2)
response(85-89)result(22-26)damus/Damoose/NIP07/SignerBridgeStorage.swift (2)
storeRequest(60-76)getResult(148-159)
DamusNostrSigner/Resources/content.js (3)
DamusNostrSigner/Resources/injected.js (3)
e(14-14)e(30-30)requestId(9-9)nostrdb/NdbTagElem.swift (1)
id(128-131)DamusNostrSigner/Resources/background.js (3)
response(31-34)response(50-50)response(80-83)
damus/Damoose/ClientPermissions.swift (1)
damus/Damoose/SigningPolicyManager.swift (1)
recordRequest(227-234)
damus/Damoose/SigningPolicy.swift (2)
damus/Damoose/Policies/ContactListPolicy.swift (1)
evaluate(36-64)damus/Damoose/SigningPolicyManager.swift (1)
evaluate(96-142)
Damoose/Resources/background.js (1)
Damoose/Resources/popup.js (2)
message(6-6)message(69-75)
damus/Damoose/SigningPolicyManager.swift (2)
damus/Damoose/ClientPermissions.swift (4)
loadAll(143-151)isKindApproved(106-123)recordRequest(126-129)save(175-179)damus/Damoose/Policies/ContactListPolicy.swift (1)
evaluate(36-64)
DamusNostrSigner/Resources/background.js (2)
DamusNostrSigner/Resources/content.js (4)
e(18-18)e(41-41)result(22-26)response(85-89)DamusNostrSigner/Resources/injected.js (3)
e(14-14)e(30-30)requestId(9-9)
damus/Damoose/Policies/ContactListPolicy.swift (1)
damus/Damoose/SigningPolicyManager.swift (1)
evaluate(96-142)
🪛 Biome (2.1.2)
Damoose/Resources/background.js
[error] 5-9: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🪛 markdownlint-cli2 (0.18.1)
damus/Damoose/DIP-05.md
21-21: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
57-57: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
104-104: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 SwiftLint (0.57.0)
damus/Damoose/NIP55/NostrSignerHandler.swift
[Warning] 96-96: TODOs should be resolved (Implement encryption/decryptio...)
(todo)
damus/Damoose/Policies/ContactListPolicy.swift
[Warning] 53-53: TODOs should be resolved (Compare against current contac...)
(todo)
damus/Damoose/NIP55/NostrSignerRequest.swift
[Warning] 148-148: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
|
|
||
| # Use bd merge for beads JSONL files | ||
| .beads/issues.jsonl merge=beads |
There was a problem hiding this comment.
Verify merge driver configuration and PR scope.
Two concerns:
-
Missing setup documentation: The custom merge driver
merge=beadsrequires git configuration (git config merge.beads.driver '...') which is not documented in this PR. Users will encounter merge failures without this setup. -
Scope concern: This file and other Beads-related changes (.beads/.gitignore, .beads/README.md, AGENTS.md bd sync workflow) introduce an issue-tracking system completely unrelated to the stated PR objectives (Damoose NIP-07/DIP-05 signing solution). Consider splitting infrastructure changes into a separate PR for clearer review and rollback capability.
📋 Suggested verification
#!/bin/bash
# Check if beads merge driver setup is documented anywhere in the PR
rg -i "merge.*beads.*driver" --type md
rg -i "git config.*beads" --type md🤖 Prompt for AI Agents
In @.gitattributes around lines 1-3, The .gitattributes entry adds a custom
merge driver (merge=beads) but the PR lacks any git configuration or
documentation for that driver and also mixes unrelated Beads infra changes into
a feature PR; add explicit setup docs showing the required git config (e.g., the
merge.beads.driver command and any helper script usage) in .beads/README.md or a
CONTRIBUTING/AGENTS.md section, and split the Beads-specific files
(.gitattributes, .beads/, .beads/README.md, .beads/.gitignore, AGENTS.md bd sync
workflow) into a separate infrastructure PR so the Damoose NIP-07/DIP-05 signing
solution PR only contains signing-related changes.
| function setup_highlighter() { | ||
| browser.contextMenus.create( | ||
| { | ||
| id: "damoose-highlighter", | ||
| title: browser.i18n.getMessage("menuHighlight"), | ||
| contexts: ["selection", "image", "link"], | ||
| }, | ||
| handle_menu_item_click | ||
| ); | ||
|
|
||
| function handle_menu_item_click(event) { | ||
| let value; | ||
| let { selectionText, srcUrl, mediaType, linkUrl, pageUrl } = event.userInfo; | ||
|
|
||
| if (mediaType === 'image') { | ||
| value = srcUrl; | ||
| } else { | ||
| value = linkUrl || selectionText; | ||
| } | ||
|
|
||
| browser.runtime.sendNativeMessage("damoose", { | ||
| kind: "highlight", | ||
| payload: { mediaType, value, selectionText, pageUrl } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
browser.contextMenus.create WebExtensions API parameters callback
💡 Result:
Short answer — browser.contextMenus.create takes two arguments:
-
createProperties (object) — properties for the menu item (common fields):
- id (string) — optional unique id (required for event pages)
- title (string) — text shown (required unless type is "separator")
- type ("normal"|"checkbox"|"radio"|"separator")
- checked (boolean) — for checkbox/radio
- contexts (string[]) — where to show (e.g. "page","selection","image", etc.)
- parentId (string|number)
- enabled (boolean)
- documentUrlPatterns (string[])
- targetUrlPatterns (string[])
- command (string) — special built-in commands (Firefox)
- onclick (function) — click handler (not allowed in event pages)
- (other browser-specific/compat fields may exist)
-
callback (function) — optional function called when creation completes. Signature: () => void. If creation failed, check runtime.lastError inside the callback. The function returns the new item's ID (string or integer). [1][2]
References: MDN contextMenus.create and Chrome docs. [1] (devdoc.net)
Citations:
🌐 Web query:
browser.contextMenus.onClicked.addListener WebExtensions callback parameters
💡 Result:
Signature:
browser.contextMenus.onClicked.addListener((info, tab) => { … })
Parameters:
-
info — OnClickData: object with details about the clicked menu item and context (common properties: menuItemId, parentMenuItemId, mediaType, linkUrl, srcUrl, pageUrl, frameUrl, frameId, selectionText, editable, wasChecked, checked, and others provided when available/permissioned). [1][2]
-
tab — tabs.Tab (optional): the tab where the click occurred; absent if the click didn’t happen in a tab. [1][2]
References: MDN (contextMenus/menus onClicked) and Chrome Extensions docs. [1][2]
Context menu click handler is incorrectly wired.
The browser.contextMenus.create() second parameter is a completion callback (called when the menu item is created), not the click handler. Click events must be handled via browser.contextMenus.onClicked.addListener(). Additionally, the code destructures event.userInfo which does not exist—the click handler receives an info object with direct properties like selectionText, srcUrl, mediaType, linkUrl, and pageUrl.
🔎 Proposed fix
function setup_highlighter() {
browser.contextMenus.create(
{
id: "damoose-highlighter",
title: browser.i18n.getMessage("menuHighlight"),
contexts: ["selection", "image", "link"],
- },
- handle_menu_item_click
+ }
);
+
+ browser.contextMenus.onClicked.addListener(handle_menu_item_click);
+}
- function handle_menu_item_click(event) {
+function handle_menu_item_click(info, tab) {
let value;
- let { selectionText, srcUrl, mediaType, linkUrl, pageUrl } = event.userInfo;
+ let { selectionText, srcUrl, mediaType, linkUrl, pageUrl } = info;
if (mediaType === 'image') {
value = srcUrl;
} else {
value = linkUrl || selectionText;
}
browser.runtime.sendNativeMessage("damoose", {
kind: "highlight",
payload: { mediaType, value, selectionText, pageUrl }
})
}
-}
setup_highlighter()📝 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.
| function setup_highlighter() { | |
| browser.contextMenus.create( | |
| { | |
| id: "damoose-highlighter", | |
| title: browser.i18n.getMessage("menuHighlight"), | |
| contexts: ["selection", "image", "link"], | |
| }, | |
| handle_menu_item_click | |
| ); | |
| function handle_menu_item_click(event) { | |
| let value; | |
| let { selectionText, srcUrl, mediaType, linkUrl, pageUrl } = event.userInfo; | |
| if (mediaType === 'image') { | |
| value = srcUrl; | |
| } else { | |
| value = linkUrl || selectionText; | |
| } | |
| browser.runtime.sendNativeMessage("damoose", { | |
| kind: "highlight", | |
| payload: { mediaType, value, selectionText, pageUrl } | |
| }) | |
| } | |
| } | |
| function setup_highlighter() { | |
| browser.contextMenus.create( | |
| { | |
| id: "damoose-highlighter", | |
| title: browser.i18n.getMessage("menuHighlight"), | |
| contexts: ["selection", "image", "link"], | |
| } | |
| ); | |
| browser.contextMenus.onClicked.addListener(handle_menu_item_click); | |
| } | |
| function handle_menu_item_click(info, tab) { | |
| let value; | |
| let { selectionText, srcUrl, mediaType, linkUrl, pageUrl } = info; | |
| if (mediaType === 'image') { | |
| value = srcUrl; | |
| } else { | |
| value = linkUrl || selectionText; | |
| } | |
| browser.runtime.sendNativeMessage("damoose", { | |
| kind: "highlight", | |
| payload: { mediaType, value, selectionText, pageUrl } | |
| }) | |
| } |
🤖 Prompt for AI Agents
In @Damoose/Resources/background.js around lines 25-50, The context menu click
handler is wired incorrectly: remove the second argument from
browser.contextMenus.create in setup_highlighter and instead register the click
handler with browser.contextMenus.onClicked.addListener(handle_menu_item_click);
update handle_menu_item_click to accept (info, tab) and destructure
selectionText, srcUrl, mediaType, linkUrl, pageUrl directly from info (not
event.userInfo), compute value = (mediaType === 'image' ? srcUrl : (linkUrl ||
selectionText)), and then call browser.runtime.sendNativeMessage("damoose", {
kind: "highlight", payload: { mediaType, value, selectionText, pageUrl } }).
| "background": { | ||
| "scripts": [ "background.js" ], | ||
| "type": "module", | ||
| "run_at": "document_end" | ||
| }, |
There was a problem hiding this comment.
Remove invalid run_at field from background script configuration.
The run_at field is only valid for content_scripts, not for background scripts in Manifest V3. Background scripts run as service workers and don't support this field. This will be ignored by the browser but indicates a misunderstanding of the MV3 architecture.
🔎 Proposed fix
"background": {
"scripts": [ "background.js" ],
- "type": "module",
- "run_at": "document_end"
+ "type": "module"
},📝 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.
| "background": { | |
| "scripts": [ "background.js" ], | |
| "type": "module", | |
| "run_at": "document_end" | |
| }, | |
| "background": { | |
| "scripts": [ "background.js" ], | |
| "type": "module" | |
| }, |
🤖 Prompt for AI Agents
In @Damoose/Resources/manifest.json around lines 17-21, Remove the invalid
"run_at" field from the "background" entry in the manifest (the object
containing "scripts": ["background.js"] and "type": "module");
background/service worker entries in Manifest V3 do not support "run_at", so
delete that property and leave the "background" object with only valid keys
(e.g., "scripts" and "type") to conform to MV3.
| function update_view(host, rs) { | ||
| const reqs = document.getElementById("requests") | ||
| const groups = summarize_requests(rs) | ||
| const rendered_groups = render_request_groups(groups) | ||
|
|
||
| reqs.innerHTML = ` | ||
| <pre>${host}</pre> is requesting: | ||
| <ul> | ||
| ${rendered_groups} | ||
| </ul> | ||
| <label> | ||
| <input type="checkbox" id="remember"> Remember this permission | ||
| </label> | ||
| <div style="margin-top: 10px;"> | ||
| <button id="approve">Approve</button> | ||
| <button id="deny">Deny</button> | ||
| </div> | ||
| ` | ||
|
|
||
| document.getElementById("approve").addEventListener("click", approve) | ||
| document.getElementById("deny").addEventListener("click", deny) | ||
| } |
There was a problem hiding this comment.
XSS vulnerability: innerHTML used with unsanitized data.
The host value (line 47) and rendered_groups (line 49) are inserted into innerHTML without sanitization. A malicious page could craft a hostname or request kind containing HTML/script tags, leading to script execution within the trusted popup context.
🔎 Proposed fix using DOM APIs instead of innerHTML
function update_view(host, rs) {
const reqs = document.getElementById("requests")
const groups = summarize_requests(rs)
- const rendered_groups = render_request_groups(groups)
-
- reqs.innerHTML = `
- <pre>${host}</pre> is requesting:
- <ul>
- ${rendered_groups}
- </ul>
- <label>
- <input type="checkbox" id="remember"> Remember this permission
- </label>
- <div style="margin-top: 10px;">
- <button id="approve">Approve</button>
- <button id="deny">Deny</button>
- </div>
- `
+ reqs.textContent = '' // Clear existing content
+
+ const pre = document.createElement('pre')
+ pre.textContent = host // Safe text insertion
+ reqs.appendChild(pre)
+
+ reqs.appendChild(document.createTextNode(' is requesting:'))
+
+ const ul = document.createElement('ul')
+ for (const kind of Object.keys(groups)) {
+ const li = document.createElement('li')
+ const num = groups[kind].length > 1 ? ` x${groups[kind].length}` : ''
+ li.textContent = kind + num // Safe text insertion
+ ul.appendChild(li)
+ }
+ reqs.appendChild(ul)
+
+ const label = document.createElement('label')
+ const checkbox = document.createElement('input')
+ checkbox.type = 'checkbox'
+ checkbox.id = 'remember'
+ label.appendChild(checkbox)
+ label.appendChild(document.createTextNode(' Remember this permission'))
+ reqs.appendChild(label)
+
+ const buttonDiv = document.createElement('div')
+ buttonDiv.style.marginTop = '10px'
+ const approveBtn = document.createElement('button')
+ approveBtn.id = 'approve'
+ approveBtn.textContent = 'Approve'
+ const denyBtn = document.createElement('button')
+ denyBtn.id = 'deny'
+ denyBtn.textContent = 'Deny'
+ buttonDiv.appendChild(approveBtn)
+ buttonDiv.appendChild(denyBtn)
+ reqs.appendChild(buttonDiv)
- document.getElementById("approve").addEventListener("click", approve)
- document.getElementById("deny").addEventListener("click", deny)
+ approveBtn.addEventListener("click", approve)
+ denyBtn.addEventListener("click", deny)
}| /// Generates random bytes for schnorr signing. | ||
| func randomBytes(count: Int) -> [UInt8] { | ||
| var bytes = [UInt8](repeating: 0, count: count) | ||
| _ = SecRandomCopyBytes(kSecRandomDefault, count, &bytes) | ||
| return bytes | ||
| } |
There was a problem hiding this comment.
Check SecRandomCopyBytes return status.
The result of SecRandomCopyBytes is discarded. If random number generation fails, signing would proceed with zero-initialized bytes, which is a security risk.
🔎 Proposed fix to check for errors
func randomBytes(count: Int) -> [UInt8] {
var bytes = [UInt8](repeating: 0, count: count)
- _ = SecRandomCopyBytes(kSecRandomDefault, count, &bytes)
+ let status = SecRandomCopyBytes(kSecRandomDefault, count, &bytes)
+ guard status == errSecSuccess else {
+ os_log(.error, "SecRandomCopyBytes failed with status: %d", status)
+ // Return empty array to signal failure
+ return []
+ }
return bytes
}📝 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.
| /// Generates random bytes for schnorr signing. | |
| func randomBytes(count: Int) -> [UInt8] { | |
| var bytes = [UInt8](repeating: 0, count: count) | |
| _ = SecRandomCopyBytes(kSecRandomDefault, count, &bytes) | |
| return bytes | |
| } | |
| /// Generates random bytes for schnorr signing. | |
| func randomBytes(count: Int) -> [UInt8] { | |
| var bytes = [UInt8](repeating: 0, count: count) | |
| let status = SecRandomCopyBytes(kSecRandomDefault, count, &bytes) | |
| guard status == errSecSuccess else { | |
| os_log(.error, "SecRandomCopyBytes failed with status: %d", status) | |
| // Return empty array to signal failure | |
| return [] | |
| } | |
| return bytes | |
| } |
🤖 Prompt for AI Agents
In @Damoose/SafariWebExtensionHandler.swift around lines 177-182, The
randomBytes(count:) function currently ignores SecRandomCopyBytes' return value;
update it to check the result of SecRandomCopyBytes(kSecRandomDefault, count,
&bytes) and handle non-success (non-errSecSuccess) cases instead of returning
zeroed bytes — either throw an error (change signature to throws and propagate a
descriptive error), or abort with a clear precondition/fatalError message;
ensure the calling code handles the new error path. Use the symbol
randomBytes(count:) and the SecRandomCopyBytes call to locate and modify the
implementation.
| D73E5EFC2C6A97F4007EB227 /* DamusAppNotificationView.swift in Sources */ = {isa = PBXBuildFile; fileRef = D78CD5972B8990300014D539 /* DamusAppNotificationView.swift */; }; | ||
| D73E5EFD2C6A97F4007EB227 /* InnerTimelineView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4CE0E2B529A3ED5500DB4CA2 /* InnerTimelineView.swift */; }; | ||
| D73E5EFE2C6A97F4007EB227 /* (null) in Sources */ = {isa = PBXBuildFile; }; | ||
| D73E5EFE2C6A97F4007EB227 /* BuildFile in Sources */ = {isa = PBXBuildFile; }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and its basic properties
ls -la damus.xcodeproj/project.pbxprojRepository: damus-io/damus
Length of output: 140
🏁 Script executed:
# Examine line 1522 and surrounding context
sed -n '1515,1530p' damus.xcodeproj/project.pbxproj | cat -nRepository: damus-io/damus
Length of output: 2605
🏁 Script executed:
# Examine line 7511 and surrounding context
sed -n '7505,7520p' damus.xcodeproj/project.pbxproj | cat -nRepository: damus-io/damus
Length of output: 1271
🏁 Script executed:
# Search for the specific BuildFile ID in the entire file to understand its usage
rg -n "D73E5EFE2C6A97F4007EB227" damus.xcodeproj/project.pbxprojRepository: damus-io/damus
Length of output: 204
Remove incomplete PBXBuildFile entry without fileRef.
Line 1522 contains a PBXBuildFile entry (D73E5EFE2C6A97F4007EB227) with no fileRef attribute, making it invalid. All other BuildFile entries in this section include a fileRef pointing to a source file. This orphaned entry must be removed along with its reference at line 7511 in the Sources build phase.
🤖 Prompt for AI Agents
In @damus.xcodeproj/project.pbxproj around line 1522, Remove the orphaned
PBXBuildFile entry named D73E5EFE2C6A97F4007EB227: delete the entire
PBXBuildFile block that has isa = PBXBuildFile and no fileRef, and also remove
any reference to D73E5EFE2C6A97F4007EB227 from the PBXSourcesBuildPhase "files"
array so the build phase no longer points to the missing BuildFile; ensure no
other sections reference that identifier after removal.
| 1B0292BDC6A10318AEA6B1AA /* images */ = { | ||
| isa = PBXGroup; | ||
| children = ( | ||
| ); | ||
| name = images; | ||
| path = images; | ||
| sourceTree = "<group>"; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find manifest.json files in the project
echo "=== Finding all manifest.json files ==="
find . -name "manifest.json" -type f 2>/dev/null | head -20
echo ""
echo "=== Checking for image references in manifest.json files ==="
find . -name "manifest.json" -type f -exec grep -l "icon\|image\|images" {} \; 2>/dev/null
echo ""
echo "=== Content of manifest.json files if found ==="
find . -name "manifest.json" -type f -exec echo "--- {} ---" \; -exec cat {} \; 2>/dev/nullRepository: damus-io/damus
Length of output: 2472
🏁 Script executed:
#!/bin/bash
# Check the project structure for images and _locales directories
echo "=== Looking for images directories ==="
find . -type d -name "images" 2>/dev/null | grep -v "\.xcodeproj" | head -20
echo ""
echo "=== Looking for _locales directories ==="
find . -type d -name "_locales" 2>/dev/null | grep -v "\.xcodeproj" | head -20
echo ""
echo "=== Check actual file structure in project root and subdirectories ==="
ls -la | head -30Repository: damus-io/damus
Length of output: 2220
🏁 Script executed:
#!/bin/bash
# Let's look at the pbxproj file around the lines mentioned to understand the context better
echo "=== Examining pbxproj file around lines 3070-3077 (images group) ==="
sed -n '3065,3085p' damus.xcodeproj/project.pbxproj
echo ""
echo "=== Examining pbxproj file around lines 5279-5285 (_locales group) ==="
sed -n '5274,5294p' damus.xcodeproj/project.pbxproj
echo ""
echo "=== Examining pbxproj file around lines 5331-5337 (another images group) ==="
sed -n '5326,5346p' damus.xcodeproj/project.pbxprojRepository: damus-io/damus
Length of output: 1965
🏁 Script executed:
#!/bin/bash
# Check for any image files in DamusNostrSigner
echo "=== Files in DamusNostrSigner/Resources ==="
ls -la DamusNostrSigner/Resources/ 2>/dev/null || echo "Directory not found"
echo ""
echo "=== All files in DamusNostrSigner ==="
find DamusNostrSigner -type f 2>/dev/null
echo ""
echo "=== Files in Damoose/Resources ==="
ls -la Damoose/Resources/ 2>/dev/null
echo ""
echo "=== Files in Damoose/Resources/images ==="
ls -la Damoose/Resources/images/ 2>/dev/null
echo ""
echo "=== Files in Damoose/Resources/_locales ==="
ls -la Damoose/Resources/_locales/ 2>/dev/nullRepository: damus-io/damus
Length of output: 2270
🏁 Script executed:
#!/bin/bash
# Check pbxproj to understand which groups belong to which target/project
echo "=== Checking PBXGroup context around images groups and _locales ==="
# Look for the parent groups and project references
echo "Context for first images group (line ~3070):"
sed -n '3050,3120p' damus.xcodeproj/project.pbxproj | grep -A 20 -B 5 "1B0292BDC6A10318AEA6B1AA"
echo ""
echo "=== Context for _locales group (line ~5279) ==="
sed -n '5260,5310p' damus.xcodeproj/project.pbxproj
echo ""
echo "=== Context for second images group (line ~5331) ==="
sed -n '5310,5360p' damus.xcodeproj/project.pbxprojRepository: damus-io/damus
Length of output: 4054
DamusNostrSigner missing required icon assets referenced in manifest.json.
The DamusNostrSigner extension's manifest.json references icon files (images/icon-48.png, images/icon-96.png, images/icon-128.png) that do not exist in the filesystem. The images/ directory is missing from DamusNostrSigner/Resources/, which will cause the extension to fail loading. Create the missing DamusNostrSigner/Resources/images/ directory and add the required icon files.
Additionally, the _locales PBXGroup in the pbxproj file lacks a path attribute, preventing proper reference to the actual _locales directory structure.
🤖 Prompt for AI Agents
In @damus.xcodeproj/project.pbxproj around lines 3070-3077, The DamusNostrSigner
extension is missing the images PBXGroup and the actual icon assets referenced
by manifest.json; create DamusNostrSigner/Resources/images/ and add the files
images/icon-48.png, images/icon-96.png, images/icon-128.png (matching the
manifest.json names), then update the PBXProject so the PBXGroup with name =
images (isa = PBXGroup) includes those file references as children and ensure
they are added to the extension target’s Copy Bundle Resources; also update the
PBXGroup for name = _locales to include a path attribute pointing to the on-disk
_locales directory so Xcode can resolve the locale subfolders and their file
references.
| enum ClientPermissionsStore { | ||
| private static let storageKey = "damoose_client_permissions" | ||
|
|
||
| /// Loads all stored client permissions. | ||
| /// | ||
| /// - Returns: Dictionary of client ID to permissions. | ||
| static func loadAll() -> [String: ClientPermissions] { | ||
| guard let data = DamusUserDefaults.standard.object(forKey: storageKey) as? Data else { | ||
| return [:] | ||
| } | ||
| guard let permissions = try? JSONDecoder().decode([String: ClientPermissions].self, from: data) else { | ||
| return [:] | ||
| } | ||
| return permissions | ||
| } | ||
|
|
||
| /// Saves all client permissions. | ||
| /// | ||
| /// - Parameter permissions: Dictionary of client ID to permissions. | ||
| static func saveAll(_ permissions: [String: ClientPermissions]) { | ||
| guard let data = try? JSONEncoder().encode(permissions) else { | ||
| return | ||
| } | ||
| DamusUserDefaults.standard.set(data, forKey: storageKey) | ||
| } | ||
|
|
||
| /// Loads permissions for a specific client. | ||
| /// | ||
| /// - Parameter clientId: The client ID to look up. | ||
| /// - Returns: The client's permissions, or nil if not found. | ||
| static func load(clientId: String) -> ClientPermissions? { | ||
| let all = loadAll() | ||
| return all[clientId] | ||
| } | ||
|
|
||
| /// Saves permissions for a specific client. | ||
| /// | ||
| /// - Parameter permissions: The permissions to save. | ||
| static func save(_ permissions: ClientPermissions) { | ||
| var all = loadAll() | ||
| all[permissions.clientId] = permissions | ||
| saveAll(all) | ||
| } | ||
|
|
||
| /// Deletes permissions for a specific client. | ||
| /// | ||
| /// - Parameter clientId: The client ID to delete. | ||
| static func delete(clientId: String) { | ||
| var all = loadAll() | ||
| all.removeValue(forKey: clientId) | ||
| saveAll(all) | ||
| } | ||
|
|
||
| /// Resets all client permissions. | ||
| static func reset() { | ||
| DamusUserDefaults.standard.removeObject(forKey: storageKey) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consider using nostrdb for permission persistence.
The current implementation uses UserDefaults for storing client permissions. Per the coding guidelines, you should maximize usage of nostrdb facilities for persistence in the Damus iOS app rather than introducing new storage mechanisms.
Consider migrating this data to nostrdb, which would provide:
- Better integration with existing Damus data infrastructure
- Consistent persistence patterns across the codebase
- Potential for more efficient queries if permissions need to be correlated with other nostr data
Based on learnings, the project prefers nostrdb-based persistence over UserDefaults for app data storage.
| static func storeRequest(eventJson: String, origin: String) -> String? { | ||
| guard let defaults = UserDefaults(suiteName: appGroup) else { | ||
| return nil | ||
| } | ||
|
|
||
| let requestId = UUID().uuidString | ||
| let request: [String: Any] = [ | ||
| "event": eventJson, | ||
| "origin": origin, | ||
| "timestamp": Date().timeIntervalSince1970 | ||
| ] | ||
|
|
||
| defaults.set(request, forKey: requestPrefix + requestId) | ||
| defaults.synchronize() | ||
|
|
||
| return requestId | ||
| } | ||
|
|
||
| /// Retrieves a pending request by ID. | ||
| /// | ||
| /// - Parameter requestId: The request ID. | ||
| /// - Returns: The request details, or nil if not found. | ||
| static func getRequest(requestId: String) -> (eventJson: String, origin: String)? { | ||
| guard let defaults = UserDefaults(suiteName: appGroup), | ||
| let request = defaults.dictionary(forKey: requestPrefix + requestId), | ||
| let eventJson = request["event"] as? String, | ||
| let origin = request["origin"] as? String else { | ||
| return nil | ||
| } | ||
|
|
||
| return (eventJson, origin) | ||
| } | ||
|
|
||
| /// Removes a pending request. | ||
| /// | ||
| /// - Parameter requestId: The request ID to remove. | ||
| static func removeRequest(requestId: String) { | ||
| guard let defaults = UserDefaults(suiteName: appGroup) else { | ||
| return | ||
| } | ||
| defaults.removeObject(forKey: requestPrefix + requestId) | ||
| defaults.synchronize() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's check the structure and find the SafariWebExtensionHandler file
find . -name "SafariWebExtensionHandler.swift" -type fRepository: damus-io/damus
Length of output: 150
🏁 Script executed:
#!/bin/bash
# Check the DamusNostrSigner/SafariWebExtensionHandler.swift file around lines 166-183
sed -n '160,190p' DamusNostrSigner/SafariWebExtensionHandler.swiftRepository: damus-io/damus
Length of output: 1113
🏁 Script executed:
#!/bin/bash
# Let's also check the getRequest and removeRequest methods in SafariWebExtensionHandler
# to see the full scope of duplication
sed -n '184,220p' DamusNostrSigner/SafariWebExtensionHandler.swiftRepository: damus-io/damus
Length of output: 1401
Refactor SafariWebExtensionHandler.storeRequest() to use SignerBridgeStorage.storeRequest().
The storeRequest method in DamusNostrSigner/SafariWebExtensionHandler.swift (lines 167-183) duplicates the logic from SignerBridgeStorage.storeRequest(). These methods are functionally identical—both create a UUID, store a dictionary with event/origin/timestamp, and synchronize. Consolidate by delegating to SignerBridgeStorage.storeRequest() instead of reimplementing.
🤖 Prompt for AI Agents
In @damus/Damoose/NIP07/SignerBridgeStorage.swift around lines 60-102,
SafariWebExtensionHandler.storeRequest() duplicates the logic of
SignerBridgeStorage.storeRequest(); replace the duplicated implementation in
SafariWebExtensionHandler.storeRequest() with a call to
SignerBridgeStorage.storeRequest(eventJson:origin:) and propagate its returned
requestId (or handle nil) instead of reimplementing UUID generation,
UserDefaults access, and synchronization to ensure single source of truth and
avoid duplicated code.
| static func cleanup() { | ||
| guard let defaults = UserDefaults(suiteName: appGroup) else { | ||
| return | ||
| } | ||
|
|
||
| let now = Date().timeIntervalSince1970 | ||
| let allKeys = defaults.dictionaryRepresentation().keys | ||
|
|
||
| for key in allKeys { | ||
| guard key.hasPrefix(requestPrefix) || key.hasPrefix(resultPrefix) else { | ||
| continue | ||
| } | ||
| guard let dict = defaults.dictionary(forKey: key), | ||
| let timestamp = dict["timestamp"] as? TimeInterval else { | ||
| continue | ||
| } | ||
| if now - timestamp > resultTTL { | ||
| defaults.removeObject(forKey: key) | ||
| } | ||
| } | ||
|
|
||
| defaults.synchronize() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'SignerBridgeStorage\.cleanup\(\)' --type swiftRepository: damus-io/damus
Length of output: 40
🏁 Script executed:
rg -n '\.cleanup\(\)' --type swiftRepository: damus-io/damus
Length of output: 141
🏁 Script executed:
cat -n damus/Damoose/NIP07/SignerBridgeStorage.swift | grep -A 5 -B 5 'cleanup'Repository: damus-io/damus
Length of output: 850
🏁 Script executed:
rg -n 'cleanup' damus --type swift -i | grep -i signerRepository: damus-io/damus
Length of output: 318
🏁 Script executed:
rg -n 'SignerBridgeStorage\.' --type swift | head -20Repository: damus-io/damus
Length of output: 1016
🏁 Script executed:
rg -n 'SignerBridgeStorage' damus --type swiftRepository: damus-io/damus
Length of output: 1238
Add automatic cleanup calls to prevent accumulation of stale entries.
The cleanup() method is never invoked in the codebase, so expired entries (older than the 5-minute TTL) will accumulate indefinitely in UserDefaults. Add periodic cleanup calls—for example, when storing a new request with storeRequest(), before retrieving results with getResult(), or on app launch—to ensure stale data is removed as intended.
🤖 Prompt for AI Agents
In @damus/Damoose/NIP07/SignerBridgeStorage.swift around lines 175-197, The
cleanup() routine is never called so stale UserDefaults entries accumulate; add
invocations of SignerBridgeStorage.cleanup() at key interaction points such as
at the start of storeRequest(...), at the start of getResult(...), and once
during app launch/initialization (e.g., when the SignerBridgeStorage singleton
or host app initializes) so expired entries are removed before storing or
reading data; ensure calls are lightweight and keep the existing implementation
unchanged.
- Fix context menu click handler: use onClicked.addListener instead of passing callback to create(), and destructure info directly (not event.userInfo) - Remove invalid run_at field from background script config (MV3) - Fix XSS vulnerability in popup.js: use DOM APIs instead of innerHTML to prevent script injection from malicious hostnames 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Refactor SafariWebExtensionHandler to use SignerBridgeStorage instead of duplicating storage logic - Add automatic cleanup() call in storeRequest() to prevent stale entry accumulation in UserDefaults - Fix randomBytes() to check SecRandomCopyBytes return status and fail gracefully instead of proceeding with zeroed bytes (security fix) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
prematurely submitted PR. closing for now |
1 participant
Summary
Damoose is a comprehensive signing solution for Damus iOS with two components:
window.nostrAPI for browser signingnostrsigner://URL scheme for app-to-app signingBoth share common infrastructure for key access, approval policies, and security.
Features
NIP-07 Safari Extension
window.nostrAPI (getPublicKey, signEvent, getRelays, nip04 encrypt/decrypt)DIP-05 URL Scheme Handler
nostrsigner://URLs from other iOS appsShared Infrastructure
SharedKeychainStorage- Cross-extension key accessSigningPolicyManager- Approval policy frameworkContactListPolicy- Protects against mass unfollowsArchitecture
Commits
bcc03395- Add NIP-55 iOS extension for external app signing (DIP-05)92e41439- Add NIP-07 Safari Web Extension (DamusNostrSigner)3c582d7f- Wire NIP-07 signEvent to DIP-05 via shared storage bridge9ae0253e- Align Safari extension App Group with main app83646928- Restore jb55's Damoose Safari extension from origin/extension3e519193- Wire native handler to keychain and implement signEvent delegationb3d88734- Add docstrings and remove dead code0c72b40d- Enable NIP-07 signer in content scripta5cea2e9- Implement direct schnorr signing in extensionb44df632- Add SigningPolicyManager integration and Safari extension setup UI1ffb9a34- Fix background.js contextMenus.create syntaxa1c7bf27- Add message origin validation in content.js (security)d05c027b- Fix popup.js payload structure mismatch676994ce- Fix approval sheet nil handling for extension requests629e730e- Fix native messaging identifier in DamusNostrSignerSecurity
Test plan
nostrsigner://URL from another app🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Infrastructure
✏️ Tip: You can customize this high-level summary in your review settings.