Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds three mobile tooling features (iOS certificate generator, Android keystore generator, iOS UDID finder) with libraries for signing/UDID handling, API routes, frontend pages/components, client download utilities, node-forge dependency, new env vars for UDID certificate signing, and documentation for certificate-backed UDID setup. Changes
Sequence Diagram(s)sequenceDiagram
actor User as User
participant Page as Tool Page
participant API as API Route
participant Lib as Signing Library
participant Forge as node-forge
participant Browser as Browser
User->>Page: submit form
Page->>API: POST JSON (input)
API->>Lib: normalizeInput() / createBundle()
Lib->>Forge: generate keypair & sign
Forge-->>Lib: signed artifacts
Lib-->>API: bundle (files, fingerprints)
API-->>Page: 200 JSON
Page->>Browser: render summary + download buttons
User->>Page: click download
Page->>Browser: trigger file download
sequenceDiagram
actor User as User
participant Finder as UDID Finder Page
participant ProfileAPI as GET /api/.../profile
participant CreateLib as createUdidMobileconfig()
participant SignLib as signMobileconfig()
participant Device as iOS Device
participant CallbackAPI as POST /api/.../callback
participant ParseLib as parseUdidDevicePayload()
participant Result as Result Page
User->>Finder: Click "Download Profile"
Finder->>ProfileAPI: GET /api/.../profile
ProfileAPI->>CreateLib: build plist
CreateLib-->>ProfileAPI: plist XML
ProfileAPI->>SignLib: sign with PEM
SignLib-->>ProfileAPI: signed mobileconfig
ProfileAPI-->>Device: serve mobileconfig
Device->>CallbackAPI: POST installed profile data
CallbackAPI->>ParseLib: extract & parse plist
ParseLib-->>CallbackAPI: UdidDevicePayload
CallbackAPI-->>Result: 302 redirect with cookie
Result-->>User: display device identifiers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ab814ac3e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Member
Author
|
@coderabbitai review |
|
Contributor
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b361c977fd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+23
to
+24
| const payload = parseUdidDevicePayload(plistXml) | ||
| const encoded = encodePayload(payload) |
There was a problem hiding this comment.
The profile generator includes a challenge (src/pages/api/tools/ios-udid-finder/profile.ts) but the callback handler accepts any parsed plist and immediately stores it in the result cookie. Because there is no challenge validation here, any client can POST a forged plist to this endpoint and make the result page display attacker-controlled identifiers, which breaks the integrity of the UDID collection flow. Validate payload.challenge (and reject mismatches) before redirecting.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (5)
src/components/tools/ToolCatalog.astro (1)
28-44: Expose the tool grid as navigation.This section is all page-to-page links, but it currently renders as a generic container. Wrapping the grid in a
<nav>landmark makes the tool switcher much easier to discover in assistive-tech landmark navigation.Small semantic improvement
- <div class="grid gap-5 md:grid-cols-3"> + <nav aria-label={title} class="grid gap-5 md:grid-cols-3"> { toolCatalog .filter((tool) => tool.slug !== current) .map((tool) => ( <a href={getRelativeLocaleUrl(Astro.locals.locale, tool.href)} class="group rounded-3xl border border-white/10 bg-white/5 p-6 transition hover:-translate-y-1 hover:border-cyan-400/50 hover:bg-white/8" > <p class="text-xs font-semibold tracking-[0.25em] text-cyan-300 uppercase">{tool.eyebrow}</p> <h3 class="mt-3 text-2xl font-bold text-white">{tool.name}</h3> <p class="mt-3 text-sm leading-7 text-slate-300">{tool.summary}</p> <span class="mt-5 inline-flex items-center text-sm font-semibold text-cyan-200 transition group-hover:text-white">Open tool</span> </a> )) } - </div> + </nav>As per coding guidelines, "Use
<main>landmark for primary content,<nav>for navigation sections,<header>and<footer>appropriately, and<section>with proper headings for semantic structure".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/tools/ToolCatalog.astro` around lines 28 - 44, The tool grid is purely navigational but is rendered as a generic <div>; wrap the grid container in a semantic <nav> landmark (with an appropriate accessible label) so assistive tech can discover it; update the block that renders toolCatalog.filter(...).map(...) (and the outer <div class="grid gap-5 md:grid-cols-3">) to be wrapped in a <nav aria-label="Tool switcher"> (or similar) while preserving the existing href generation via getRelativeLocaleUrl(Astro.locals.locale, tool.href) and excluding current by tool.slug !== current.src/pages/tools/ios-udid-finder/result.astro (1)
13-17: Missing JSON-LD structured data for SEO.The page defines
titleanddescriptionbut omits theldJSONproperty. Even though this page usesrobots: 'noindex, nofollow', other tool pages in this PR include JSON-LD for consistency. Consider adding at minimum acreateWebPageLdJsonentity.As per coding guidelines: "Include JSON-LD structured data using
src/lib/ldJson.tshelpers for SEO"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/tools/ios-udid-finder/result.astro` around lines 13 - 17, Add JSON-LD structured data to the page by importing the createWebPageLdJson helper from src/lib/ldJson.ts and adding an ldJSON (or ldJson) property to the content object; use createWebPageLdJson({ title, description, url: /* page URL or canonical */ }) to build the payload and assign it to content.ldJSON so the page mirrors other tool pages' structured data even with robots: 'noindex, nofollow'.src/lib/tools/signing.ts (3)
106-112: Validation catches edge cases but could be cleaner.If
valueis an object,parseInt('[object Object]')returnsNaN, which fails validation correctly. However, for clarity and to match the pattern incleanField, an explicit type guard would be cleaner.♻️ Optional improvement
function validateValidityYears(value: unknown): number { - const parsed = Number.parseInt(String(value ?? '10'), 10) + const raw = typeof value === 'number' ? value : typeof value === 'string' ? value : '10' + const parsed = Number.parseInt(String(raw), 10) if (!Number.isFinite(parsed) || parsed < 1 || parsed > 35) { throw new Error('Validity must be between 1 and 35 years.') } return parsed }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/tools/signing.ts` around lines 106 - 112, The validateValidityYears function currently relies on parseInt to reject objects but should use an explicit type guard similar to cleanField; update validateValidityYears to first accept undefined (default to '10'), allow only number or string inputs (if number use Math.floor or Number conversion, if string use parseInt), and throw a clear error for other types (e.g., objects/arrays), then validate the parsed integer is finite and between 1 and 35 before returning it.
249-253: Consider noting 3DES deprecation for PKCS#12.The PKCS#12 keystore uses
algorithm: '3des'which is supported by Android tooling but is a legacy algorithm. For future consideration, newer PKCS#12 implementations prefer AES-based encryption. This is not blocking since Android build tools still expect 3DES compatibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/tools/signing.ts` around lines 249 - 253, The code currently calls forge.pkcs12.toPkcs12Asn1(..., { algorithm: '3des', friendlyName: input.alias, ... }) which forces 3DES; add a brief inline comment near this call (and/or the pkcs12 variable) noting that 3DES is legacy/deprecated and kept for Android tooling compatibility, and make the algorithm configurable (e.g., expose an option on the input or a constant used by signing.ts) so callers can opt into AES-based PKCS#12 in the future; ensure references are to forge.pkcs12.toPkcs12Asn1, the pkcs12 variable, input.password, input.alias, keyPair.privateKey, and certificate so the change is localized and backwards-compatible by default.
59-64: UsereplaceAlland consider type guard for robustness.Per static analysis, prefer
String#replaceAll()overString#replace()with global regex. While form entries are typically strings, a defensive type check avoids'[object Object]'edge cases.♻️ Suggested improvement
function cleanField(value: unknown, maxLength = MAX_FIELD_LENGTH): string { + if (typeof value === 'object' && value !== null) { + return '' + } return String(value ?? '') .trim() - .replace(/\s+/g, ' ') + .replaceAll(/\s+/g, ' ') .slice(0, maxLength) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/tools/signing.ts` around lines 59 - 64, The cleanField function should defensively handle non-string inputs and avoid producing "[object Object]" and should use replaceAll instead of a global regex; update cleanField to first type-guard: if value is a number convert to string, if it's not a string/number return '' (or empty string), then trim and collapse repeated whitespace using a replaceAll-based approach (e.g., repeatedly replaceAll(' ', ' ') until no double-spaces) or an equivalent replaceAll strategy, and finally slice to MAX_FIELD_LENGTH; reference the cleanField function and MAX_FIELD_LENGTH when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/ios-udid-finder-certificate-setup.md`:
- Around line 62-68: Update the runbook to clarify that installing the profile
on a real iPhone/iPad requires the device to be able to reach the dev host:
modify step 6 to state that the URL serving /api/tools/ios-udid-finder/profile
(and the "Download trust certificate" button controlled by
PUBLIC_IOS_UDID_CERTIFICATE_LINK) must be accessible from the phone—either by
using a LAN-accessible host/IP or a tunneling service (ngrok/localhost.run) so
the device can fetch the profile successfully.
In `@src/components/Footer.astro`:
- Around line 67-70: The footer menu item currently hardcodes the English label
"Free Mobile Tools" in the object with property name (see the entry using
getRelativeLocaleUrl), breaking localization; replace that literal with a lookup
into the message catalog (e.g., use the app's messages/translation helper
available via Astro.locals or the project's intl helper) so the label is sourced
from a new key like footer.freeMobileTools in the message catalog and used in
place of the hardcoded string in the Footer.astro menu item.
In `@src/lib/tools/udid.ts`:
- Around line 88-123: The signing code using getForge(), certificatePem,
privateKeyPem, chainPem and signedData.sign(...) can throw on malformed PEMs;
wrap the certificate/key parsing and signing steps (the block that creates
signingCertificate, privateKey, adds certificates/signers and calls
signedData.sign) in a try/catch and return null on any error so signing is
fail-soft; preserve the existing unsigned path by returning Buffer as now when
successful, but if an exception occurs catch it and return null (reference
symbols: getForge, certificateFromPem, privateKeyFromPem,
signedData.addCertificate, signedData.addSigner, signedData.sign).
In `@src/pages/api/tools/android-keystore-generator.ts`:
- Around line 9-18: The POST handler currently collapses all exceptions into a
400 response; update the catch in the POST function to distinguish
validation/input errors from unexpected keystore generation failures by checking
the error type or a sentinel (e.g., errors thrown by
normalizeAndroidKeystoreInput vs those from createAndroidKeystoreBundle), return
webJson(..., 400, headers) for known/validation errors and webJson({ error:
'Internal server error' }, 500, headers) for unexpected failures from
createAndroidKeystoreBundle(), and ensure the unexpected error is logged (not
sent to the client) so monitoring captures 5xxs while clients only receive a
generic message.
In `@src/pages/api/tools/ios-certificate-generator.ts`:
- Around line 9-18: The POST handler currently maps all exceptions to a 400 with
the raw error message; change it to keep input/validation errors (thrown by
normalizeIosCertificateInput or a dedicated InputValidationError) returning
webJson(..., 400, headers) but treat any other exception from
createIosCertificateBundle (or unexpected errors inside POST) as a 500 with a
generic client-facing message (e.g., "Internal server error generating iOS
certificate") while logging the original error server-side (console.error or
your app logger) so internals are not exposed; update the catch in POST to
distinguish error types (e.g., instanceof InputValidationError or specific
validation checks) and branch to return 400 vs. 500 accordingly, referencing
POST, normalizeIosCertificateInput, createIosCertificateBundle, and webJson.
In `@src/pages/api/tools/ios-udid-finder/callback.ts`:
- Around line 21-24: The redirect currently exposes sensitive device data
because parseUdidDevicePayload(...) + encodePayload(...) are placed into the
query string via webRedirect(...), so change the handoff to use a short-lived
opaque server-side token: generate a random token (e.g., UUID or secure random
string) in the callback handler, store the encoded payload returned by
encodePayload(...) in a server-side store/cache with a TTL (in-memory cache,
Redis, or DB) keyed by that token, then call webRedirect to
/tools/ios-udid-finder/result/?token=... (only the token in the URL). Update the
result handler to fetch and delete the payload by token from the same store, and
ensure no device identifiers are logged or stored in logs/access entries.
In `@src/pages/tools/index.astro`:
- Around line 18-35: The itemList and Home breadcrumb are built from the default
baseUrl so structured-data points to the wrong locale; update the URL
construction in createItemListLdJson (the toolCatalog.map that uses new
URL(`/${tool.href}/`, Astro.locals.runtimeConfig.public.baseUrl)) to derive
absolute URLs from the current page locale (use Astro.url.origin or
Astro.url.href + locale-aware path) and likewise update the Home breadcrumb
passed into createLdJsonGraph (replace Astro.locals.runtimeConfig.public.baseUrl
with a locale-aware absolute root built from Astro.url) so both itemList entries
and the Home breadcrumb use the current locale when generating schema URLs.
In `@src/pages/tools/ios-udid-finder/result.astro`:
- Around line 91-105: Wrap the clipboard write in a try/catch inside the click
handler created by document.querySelectorAll('[data-copy-value]') so
navigator.clipboard.writeText(value) failures are caught; on success set the
button.textContent to "Copied" and push a short text update to an aria-live
region (create or reuse a hidden element with aria-live="polite") so screen
readers announce the status, then restore the original text after the timeout;
on failure, set the button.textContent to an error message (e.g., "Copy
failed"), update the aria-live region with the failure reason, and optionally
attempt a fallback (document.execCommand('copy')) before showing the error,
ensuring original text is restored and no unhandled rejections occur.
In `@src/scripts/tool-client.ts`:
- Around line 40-48: The code currently does response.json().catch(() => null)
and then returns payload as TResponse even when payload is null; add a guard
after parsing to detect payload === null and throw a clear error (e.g., "Invalid
or empty JSON response") before returning, so successful 2xx responses with
invalid JSON don't propagate a null TResponse; reference the payload variable,
the response.json() call, response.ok check, and the returned TResponse so you
can locate and modify that block.
- Around line 23-29: The URL is revoked immediately after link.click(), which
can race the browser's download start; update the download flow (the block
creating url, link.href, link.download and calling link.click()) to delay the
URL.revokeObjectURL(url) call—e.g., use a short setTimeout or an appropriate
event handler to revoke the object URL after the browser has begun the
download—so that URL.createObjectURL / link.click() are used as-is but
URL.revokeObjectURL(url) is deferred to avoid intermittent failures.
---
Nitpick comments:
In `@src/components/tools/ToolCatalog.astro`:
- Around line 28-44: The tool grid is purely navigational but is rendered as a
generic <div>; wrap the grid container in a semantic <nav> landmark (with an
appropriate accessible label) so assistive tech can discover it; update the
block that renders toolCatalog.filter(...).map(...) (and the outer <div
class="grid gap-5 md:grid-cols-3">) to be wrapped in a <nav aria-label="Tool
switcher"> (or similar) while preserving the existing href generation via
getRelativeLocaleUrl(Astro.locals.locale, tool.href) and excluding current by
tool.slug !== current.
In `@src/lib/tools/signing.ts`:
- Around line 106-112: The validateValidityYears function currently relies on
parseInt to reject objects but should use an explicit type guard similar to
cleanField; update validateValidityYears to first accept undefined (default to
'10'), allow only number or string inputs (if number use Math.floor or Number
conversion, if string use parseInt), and throw a clear error for other types
(e.g., objects/arrays), then validate the parsed integer is finite and between 1
and 35 before returning it.
- Around line 249-253: The code currently calls forge.pkcs12.toPkcs12Asn1(..., {
algorithm: '3des', friendlyName: input.alias, ... }) which forces 3DES; add a
brief inline comment near this call (and/or the pkcs12 variable) noting that
3DES is legacy/deprecated and kept for Android tooling compatibility, and make
the algorithm configurable (e.g., expose an option on the input or a constant
used by signing.ts) so callers can opt into AES-based PKCS#12 in the future;
ensure references are to forge.pkcs12.toPkcs12Asn1, the pkcs12 variable,
input.password, input.alias, keyPair.privateKey, and certificate so the change
is localized and backwards-compatible by default.
- Around line 59-64: The cleanField function should defensively handle
non-string inputs and avoid producing "[object Object]" and should use
replaceAll instead of a global regex; update cleanField to first type-guard: if
value is a number convert to string, if it's not a string/number return '' (or
empty string), then trim and collapse repeated whitespace using a
replaceAll-based approach (e.g., repeatedly replaceAll(' ', ' ') until no
double-spaces) or an equivalent replaceAll strategy, and finally slice to
MAX_FIELD_LENGTH; reference the cleanField function and MAX_FIELD_LENGTH when
making the change.
In `@src/pages/tools/ios-udid-finder/result.astro`:
- Around line 13-17: Add JSON-LD structured data to the page by importing the
createWebPageLdJson helper from src/lib/ldJson.ts and adding an ldJSON (or
ldJson) property to the content object; use createWebPageLdJson({ title,
description, url: /* page URL or canonical */ }) to build the payload and assign
it to content.ldJSON so the page mirrors other tool pages' structured data even
with robots: 'noindex, nofollow'.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 80c03613-24c6-42ed-a33a-29255e5193ba
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
.env.exampleastro.config.mjsdocs/ios-udid-finder-certificate-setup.mdpackage.jsonsrc/components/Footer.astrosrc/components/tools/ToolCatalog.astrosrc/components/tools/ToolFaq.astrosrc/config/plugins.tssrc/lib/tools/catalog.tssrc/lib/tools/signing.tssrc/lib/tools/udid.tssrc/pages/api/tools/android-keystore-generator.tssrc/pages/api/tools/ios-certificate-generator.tssrc/pages/api/tools/ios-udid-finder/callback.tssrc/pages/api/tools/ios-udid-finder/profile.tssrc/pages/tools/android-keystore-generator/index.astrosrc/pages/tools/index.astrosrc/pages/tools/ios-certificate-generator/index.astrosrc/pages/tools/ios-udid-finder/index.astrosrc/pages/tools/ios-udid-finder/result.astrosrc/scripts/tool-client.ts
Comment on lines
+62
to
+68
| 1. Start the site locally with the environment variables set. | ||
| 2. Open `/tools/ios-udid-finder/`. | ||
| 3. Confirm the page shows the "Download trust certificate" button if `PUBLIC_IOS_UDID_CERTIFICATE_LINK` is present. | ||
| 4. Download the profile from `/api/tools/ios-udid-finder/profile`. | ||
| 5. Confirm the response header is: | ||
| - `Content-Type: application/x-apple-aspen-config` | ||
| 6. Install the profile on a real iPhone or iPad. |
Contributor
There was a problem hiding this comment.
Real-device verification needs a phone-reachable host.
These steps read like a local dev server is enough, but the phone has to fetch /api/tools/ios-udid-finder/profile itself. Call out that step 6 needs a LAN-accessible or tunneled URL; otherwise the runbook is hard to follow successfully.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/ios-udid-finder-certificate-setup.md` around lines 62 - 68, Update the
runbook to clarify that installing the profile on a real iPhone/iPad requires
the device to be able to reach the dev host: modify step 6 to state that the URL
serving /api/tools/ios-udid-finder/profile (and the "Download trust certificate"
button controlled by PUBLIC_IOS_UDID_CERTIFICATE_LINK) must be accessible from
the phone—either by using a LAN-accessible host/IP or a tunneling service
(ngrok/localhost.run) so the device can fetch the profile successfully.
Comment on lines
+67
to
+70
| { | ||
| name: 'Free Mobile Tools', | ||
| href: getRelativeLocaleUrl(Astro.locals.locale, 'tools'), | ||
| }, |
Contributor
There was a problem hiding this comment.
Localize the new footer label.
Line 68 introduces a hardcoded English string, while this menu is otherwise locale-driven. Please move this text to the message catalog to keep translations consistent.
💡 Suggested fix
- name: 'Free Mobile Tools',
+ name: m.free_mobile_tools({}, { locale: Astro.locals.locale }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Footer.astro` around lines 67 - 70, The footer menu item
currently hardcodes the English label "Free Mobile Tools" in the object with
property name (see the entry using getRelativeLocaleUrl), breaking localization;
replace that literal with a lookup into the message catalog (e.g., use the app's
messages/translation helper available via Astro.locals or the project's intl
helper) so the label is sourced from a new key like footer.freeMobileTools in
the message catalog and used in place of the hardcoded string in the
Footer.astro menu item.
Comment on lines
+88
to
+123
| const forge = await getForge() | ||
| const signingCertificate = forge.pki.certificateFromPem(certificatePem) | ||
| const privateKey = forge.pki.privateKeyFromPem(privateKeyPem) | ||
| const signedData = forge.pkcs7.createSignedData() | ||
| signedData.content = forge.util.createBuffer(profileXml, 'utf8') | ||
| signedData.addCertificate(signingCertificate) | ||
|
|
||
| if (chainPem) { | ||
| const matches = chainPem.match(/-----BEGIN CERTIFICATE-----[\s\S]+?-----END CERTIFICATE-----/g) || [] | ||
| for (const certificate of matches) { | ||
| signedData.addCertificate(forge.pki.certificateFromPem(certificate)) | ||
| } | ||
| } | ||
|
|
||
| signedData.addSigner({ | ||
| key: privateKey, | ||
| certificate: signingCertificate, | ||
| digestAlgorithm: forge.pki.oids.sha256, | ||
| authenticatedAttributes: [ | ||
| { | ||
| type: forge.pki.oids.contentType, | ||
| value: forge.pki.oids.data, | ||
| }, | ||
| { | ||
| type: forge.pki.oids.messageDigest, | ||
| }, | ||
| { | ||
| type: forge.pki.oids.signingTime, | ||
| value: new Date(), | ||
| }, | ||
| ], | ||
| }) | ||
|
|
||
| signedData.sign({ detached: false }) | ||
| return Buffer.from(forge.asn1.toDer(signedData.toAsn1()).getBytes(), 'binary') | ||
| } |
Contributor
There was a problem hiding this comment.
Make signing fail-soft when PEM data is invalid.
Lines 88-123 can throw on malformed cert/key/chain content, which turns optional signing into a hard failure path. Return null on signing errors so callers can safely fall back to unsigned profiles.
💡 Suggested fix
export async function signMobileconfig(profileXml: string): Promise<Buffer | null> {
const certificatePem = normalizePem(import.meta.env.IOS_UDID_PROFILE_SIGNING_CERT_PEM)
const privateKeyPem = normalizePem(import.meta.env.IOS_UDID_PROFILE_SIGNING_KEY_PEM)
const chainPem = normalizePem(import.meta.env.IOS_UDID_PROFILE_SIGNING_CHAIN_PEM)
if (!certificatePem || !privateKeyPem) {
return null
}
- const forge = await getForge()
- const signingCertificate = forge.pki.certificateFromPem(certificatePem)
- const privateKey = forge.pki.privateKeyFromPem(privateKeyPem)
- const signedData = forge.pkcs7.createSignedData()
- signedData.content = forge.util.createBuffer(profileXml, 'utf8')
- signedData.addCertificate(signingCertificate)
+ try {
+ const forge = await getForge()
+ const signingCertificate = forge.pki.certificateFromPem(certificatePem)
+ const privateKey = forge.pki.privateKeyFromPem(privateKeyPem)
+ const signedData = forge.pkcs7.createSignedData()
+ signedData.content = forge.util.createBuffer(profileXml, 'utf8')
+ signedData.addCertificate(signingCertificate)
- if (chainPem) {
- const matches = chainPem.match(/-----BEGIN CERTIFICATE-----[\s\S]+?-----END CERTIFICATE-----/g) || []
- for (const certificate of matches) {
- signedData.addCertificate(forge.pki.certificateFromPem(certificate))
+ if (chainPem) {
+ const matches = chainPem.match(/-----BEGIN CERTIFICATE-----[\s\S]+?-----END CERTIFICATE-----/g) || []
+ for (const certificate of matches) {
+ signedData.addCertificate(forge.pki.certificateFromPem(certificate))
+ }
}
- }
- signedData.addSigner({
- key: privateKey,
- certificate: signingCertificate,
- digestAlgorithm: forge.pki.oids.sha256,
- authenticatedAttributes: [
- {
- type: forge.pki.oids.contentType,
- value: forge.pki.oids.data,
- },
- {
- type: forge.pki.oids.messageDigest,
- },
- {
- type: forge.pki.oids.signingTime,
- value: new Date(),
- },
- ],
- })
+ signedData.addSigner({
+ key: privateKey,
+ certificate: signingCertificate,
+ digestAlgorithm: forge.pki.oids.sha256,
+ authenticatedAttributes: [
+ { type: forge.pki.oids.contentType, value: forge.pki.oids.data },
+ { type: forge.pki.oids.messageDigest },
+ { type: forge.pki.oids.signingTime, value: new Date() },
+ ],
+ })
- signedData.sign({ detached: false })
- return Buffer.from(forge.asn1.toDer(signedData.toAsn1()).getBytes(), 'binary')
+ signedData.sign({ detached: false })
+ return Buffer.from(forge.asn1.toDer(signedData.toAsn1()).getBytes(), 'binary')
+ } catch {
+ return null
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/tools/udid.ts` around lines 88 - 123, The signing code using
getForge(), certificatePem, privateKeyPem, chainPem and signedData.sign(...) can
throw on malformed PEMs; wrap the certificate/key parsing and signing steps (the
block that creates signingCertificate, privateKey, adds certificates/signers and
calls signedData.sign) in a try/catch and return null on any error so signing is
fail-soft; preserve the existing unsigned path by returning Buffer as now when
successful, but if an exception occurs catch it and return null (reference
symbols: getForge, certificateFromPem, privateKeyFromPem,
signedData.addCertificate, signedData.addSigner, signedData.sign).
Comment on lines
+9
to
+18
| export const POST: APIRoute = async ({ request }) => { | ||
| try { | ||
| const body = await request.json() | ||
| const input = normalizeAndroidKeystoreInput(body) | ||
| const bundle = await createAndroidKeystoreBundle(input) | ||
|
|
||
| return webJson(bundle, 200, headers) | ||
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : 'Unable to generate the Android keystore.' | ||
| return webJson({ error: message }, 400, headers) |
Contributor
There was a problem hiding this comment.
Don’t collapse generator failures into a 400.
This catch treats validation errors and unexpected keystore-generation failures the same, and returns error.message in both cases. That hides real server faults from 5xx monitoring and leaks internal failure text to clients. Keep expected request errors as 400, but return a generic 500 for unanticipated createAndroidKeystoreBundle() failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/api/tools/android-keystore-generator.ts` around lines 9 - 18, The
POST handler currently collapses all exceptions into a 400 response; update the
catch in the POST function to distinguish validation/input errors from
unexpected keystore generation failures by checking the error type or a sentinel
(e.g., errors thrown by normalizeAndroidKeystoreInput vs those from
createAndroidKeystoreBundle), return webJson(..., 400, headers) for
known/validation errors and webJson({ error: 'Internal server error' }, 500,
headers) for unexpected failures from createAndroidKeystoreBundle(), and ensure
the unexpected error is logged (not sent to the client) so monitoring captures
5xxs while clients only receive a generic message.
Comment on lines
+9
to
+18
| export const POST: APIRoute = async ({ request }) => { | ||
| try { | ||
| const body = await request.json() | ||
| const input = normalizeIosCertificateInput(body) | ||
| const bundle = await createIosCertificateBundle(input) | ||
|
|
||
| return webJson(bundle, 200, headers) | ||
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : 'Unable to generate the iOS signing request.' | ||
| return webJson({ error: message }, 400, headers) |
Contributor
There was a problem hiding this comment.
Unexpected CSR-generation failures shouldn’t come back as 400.
This catch currently turns both bad input and unanticipated createIosCertificateBundle() failures into 400 responses with raw error.message. That hides real server faults from 5xx monitoring and exposes internal failure text to clients. Keep expected request errors as 400, but return a generic 500 for unexpected generation failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/api/tools/ios-certificate-generator.ts` around lines 9 - 18, The
POST handler currently maps all exceptions to a 400 with the raw error message;
change it to keep input/validation errors (thrown by
normalizeIosCertificateInput or a dedicated InputValidationError) returning
webJson(..., 400, headers) but treat any other exception from
createIosCertificateBundle (or unexpected errors inside POST) as a 500 with a
generic client-facing message (e.g., "Internal server error generating iOS
certificate") while logging the original error server-side (console.error or
your app logger) so internals are not exposed; update the catch in POST to
distinguish error types (e.g., instanceof InputValidationError or specific
validation checks) and branch to return 400 vs. 500 accordingly, referencing
POST, normalizeIosCertificateInput, createIosCertificateBundle, and webJson.
Comment on lines
+21
to
+24
| const payload = parseUdidDevicePayload(plistXml) | ||
| const encoded = encodePayload(payload) | ||
|
|
||
| return webRedirect(`/tools/ios-udid-finder/result/?payload=${encodeURIComponent(encoded)}`, 302, headers) |
Contributor
There was a problem hiding this comment.
Don’t put the device payload in the query string.
encodePayload() in src/lib/tools/udid.ts:156-178 is plain base64url JSON, so this redirect URL still carries the UDID, serial, IMEI, and MEID in browser history, access logs, referrers, and copied links. Cache-Control: no-store does not prevent that leakage. Use a short-lived opaque token or another server-side handoff instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/api/tools/ios-udid-finder/callback.ts` around lines 21 - 24, The
redirect currently exposes sensitive device data because
parseUdidDevicePayload(...) + encodePayload(...) are placed into the query
string via webRedirect(...), so change the handoff to use a short-lived opaque
server-side token: generate a random token (e.g., UUID or secure random string)
in the callback handler, store the encoded payload returned by
encodePayload(...) in a server-side store/cache with a TTL (in-memory cache,
Redis, or DB) keyed by that token, then call webRedirect to
/tools/ios-udid-finder/result/?token=... (only the token in the URL). Update the
result handler to fetch and delete the payload by token from the same store, and
ensure no device identifiers are logged or stored in logs/access entries.
Comment on lines
+18
to
+35
| const itemList = createItemListLdJson(Astro.locals.runtimeConfig.public, { | ||
| url: Astro.url.href, | ||
| name: 'Capgo mobile signing tools', | ||
| description, | ||
| items: toolCatalog.map((tool) => ({ | ||
| name: tool.name, | ||
| url: new URL(`/${tool.href}/`, Astro.locals.runtimeConfig.public.baseUrl).toString(), | ||
| description: tool.summary, | ||
| })), | ||
| }) | ||
|
|
||
| const ldJSON = createLdJsonGraph(Astro.locals.runtimeConfig.public, webPage, { | ||
| includeOrganization: true, | ||
| includeBreadcrumbs: true, | ||
| breadcrumbs: [ | ||
| { name: 'Home', url: Astro.locals.runtimeConfig.public.baseUrl }, | ||
| { name: 'Tools', url: Astro.url.href }, | ||
| ], |
Contributor
There was a problem hiding this comment.
Keep the structured-data URLs in the current locale.
The rendered links on this page are locale-aware, but the itemList URLs and the Home breadcrumb are built from default-locale paths. On translated pages, that publishes schema pointing crawlers at the wrong routes. Build those absolute URLs from the current locale too.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/tools/index.astro` around lines 18 - 35, The itemList and Home
breadcrumb are built from the default baseUrl so structured-data points to the
wrong locale; update the URL construction in createItemListLdJson (the
toolCatalog.map that uses new URL(`/${tool.href}/`,
Astro.locals.runtimeConfig.public.baseUrl)) to derive absolute URLs from the
current page locale (use Astro.url.origin or Astro.url.href + locale-aware path)
and likewise update the Home breadcrumb passed into createLdJsonGraph (replace
Astro.locals.runtimeConfig.public.baseUrl with a locale-aware absolute root
built from Astro.url) so both itemList entries and the Home breadcrumb use the
current locale when generating schema URLs.
Comment on lines
+91
to
+105
| <script> | ||
| document.querySelectorAll<HTMLButtonElement>('[data-copy-value]').forEach((button) => { | ||
| button.addEventListener('click', async () => { | ||
| const value = button.dataset.copyValue | ||
| if (!value) return | ||
|
|
||
| await navigator.clipboard.writeText(value) | ||
| const original = button.textContent || 'Copy' | ||
| button.textContent = 'Copied' | ||
| window.setTimeout(() => { | ||
| button.textContent = original | ||
| }, 1600) | ||
| }) | ||
| }) | ||
| </script> |
Contributor
There was a problem hiding this comment.
Add error handling for clipboard operations and announce status to screen readers.
The navigator.clipboard.writeText call can fail (e.g., permission denied, non-secure context). Additionally, the "Copied" feedback should be announced to assistive technologies via an aria-live region.
🛡️ Proposed fix with error handling and accessibility
<script>
document.querySelectorAll<HTMLButtonElement>('[data-copy-value]').forEach((button) => {
button.addEventListener('click', async () => {
const value = button.dataset.copyValue
if (!value) return
- await navigator.clipboard.writeText(value)
- const original = button.textContent || 'Copy'
- button.textContent = 'Copied'
- window.setTimeout(() => {
- button.textContent = original
- }, 1600)
+ try {
+ await navigator.clipboard.writeText(value)
+ const original = button.textContent || 'Copy'
+ button.textContent = 'Copied'
+ button.setAttribute('aria-live', 'polite')
+ window.setTimeout(() => {
+ button.textContent = original
+ button.removeAttribute('aria-live')
+ }, 1600)
+ } catch {
+ button.textContent = 'Failed'
+ window.setTimeout(() => {
+ button.textContent = 'Copy'
+ }, 1600)
+ }
})
})
</script>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <script> | |
| document.querySelectorAll<HTMLButtonElement>('[data-copy-value]').forEach((button) => { | |
| button.addEventListener('click', async () => { | |
| const value = button.dataset.copyValue | |
| if (!value) return | |
| await navigator.clipboard.writeText(value) | |
| const original = button.textContent || 'Copy' | |
| button.textContent = 'Copied' | |
| window.setTimeout(() => { | |
| button.textContent = original | |
| }, 1600) | |
| }) | |
| }) | |
| </script> | |
| <script> | |
| document.querySelectorAll<HTMLButtonElement>('[data-copy-value]').forEach((button) => { | |
| button.addEventListener('click', async () => { | |
| const value = button.dataset.copyValue | |
| if (!value) return | |
| try { | |
| await navigator.clipboard.writeText(value) | |
| const original = button.textContent || 'Copy' | |
| button.textContent = 'Copied' | |
| button.setAttribute('aria-live', 'polite') | |
| window.setTimeout(() => { | |
| button.textContent = original | |
| button.removeAttribute('aria-live') | |
| }, 1600) | |
| } catch { | |
| button.textContent = 'Failed' | |
| window.setTimeout(() => { | |
| button.textContent = 'Copy' | |
| }, 1600) | |
| } | |
| }) | |
| }) | |
| </script> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/tools/ios-udid-finder/result.astro` around lines 91 - 105, Wrap the
clipboard write in a try/catch inside the click handler created by
document.querySelectorAll('[data-copy-value]') so
navigator.clipboard.writeText(value) failures are caught; on success set the
button.textContent to "Copied" and push a short text update to an aria-live
region (create or reuse a hidden element with aria-live="polite") so screen
readers announce the status, then restore the original text after the timeout;
on failure, set the button.textContent to an error message (e.g., "Copy
failed"), update the aria-live region with the failure reason, and optionally
attempt a fallback (document.execCommand('copy')) before showing the error,
ensuring original text is restored and no unhandled rejections occur.
Comment on lines
+23
to
+29
| const url = URL.createObjectURL(blob) | ||
| const link = document.createElement('a') | ||
| link.href = url | ||
| link.download = file.fileName | ||
| link.click() | ||
| URL.revokeObjectURL(url) | ||
| } |
Contributor
There was a problem hiding this comment.
Delay object URL revocation to avoid intermittent download failures.
Line 28 revokes the URL immediately after click(). In some browsers, this can race the download start and cause flaky/no download behavior.
💡 Suggested fix
export function downloadFile(file: DownloadableFile): void {
const blob = file.encoding === 'base64' ? decodeBase64ToBlob(file.content, file.mimeType) : new Blob([file.content], { type: file.mimeType })
const url = URL.createObjectURL(blob)
const link = document.createElement('a')
link.href = url
link.download = file.fileName
+ document.body.append(link)
link.click()
- URL.revokeObjectURL(url)
+ link.remove()
+ setTimeout(() => URL.revokeObjectURL(url), 0)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const url = URL.createObjectURL(blob) | |
| const link = document.createElement('a') | |
| link.href = url | |
| link.download = file.fileName | |
| link.click() | |
| URL.revokeObjectURL(url) | |
| } | |
| const url = URL.createObjectURL(blob) | |
| const link = document.createElement('a') | |
| link.href = url | |
| link.download = file.fileName | |
| document.body.append(link) | |
| link.click() | |
| link.remove() | |
| setTimeout(() => URL.revokeObjectURL(url), 0) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/scripts/tool-client.ts` around lines 23 - 29, The URL is revoked
immediately after link.click(), which can race the browser's download start;
update the download flow (the block creating url, link.href, link.download and
calling link.click()) to delay the URL.revokeObjectURL(url) call—e.g., use a
short setTimeout or an appropriate event handler to revoke the object URL after
the browser has begun the download—so that URL.createObjectURL / link.click()
are used as-is but URL.revokeObjectURL(url) is deferred to avoid intermittent
failures.
Comment on lines
+40
to
+48
| const payload = await response.json().catch(() => null) | ||
|
|
||
| if (!response.ok) { | ||
| const message = payload && typeof payload.error === 'string' ? payload.error : 'The request failed. Please review your values and try again.' | ||
| throw new Error(message) | ||
| } | ||
|
|
||
| return payload as TResponse | ||
| } |
Contributor
There was a problem hiding this comment.
Guard against null payload on successful responses.
Lines 40 and 47 can return null as TResponse when JSON parsing fails on a 2xx response, which pushes a hidden runtime failure downstream.
💡 Suggested fix
const payload = await response.json().catch(() => null)
if (!response.ok) {
const message = payload && typeof payload.error === 'string' ? payload.error : 'The request failed. Please review your values and try again.'
throw new Error(message)
}
+ if (payload === null) {
+ throw new Error('The server returned an empty or invalid JSON response.')
+ }
+
return payload as TResponse
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const payload = await response.json().catch(() => null) | |
| if (!response.ok) { | |
| const message = payload && typeof payload.error === 'string' ? payload.error : 'The request failed. Please review your values and try again.' | |
| throw new Error(message) | |
| } | |
| return payload as TResponse | |
| } | |
| const payload = await response.json().catch(() => null) | |
| if (!response.ok) { | |
| const message = payload && typeof payload.error === 'string' ? payload.error : 'The request failed. Please review your values and try again.' | |
| throw new Error(message) | |
| } | |
| if (payload === null) { | |
| throw new Error('The server returned an empty or invalid JSON response.') | |
| } | |
| return payload as TResponse | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/scripts/tool-client.ts` around lines 40 - 48, The code currently does
response.json().catch(() => null) and then returns payload as TResponse even
when payload is null; add a guard after parsing to detect payload === null and
throw a clear error (e.g., "Invalid or empty JSON response") before returning,
so successful 2xx responses with invalid JSON don't propagate a null TResponse;
reference the payload variable, the response.json() call, response.ok check, and
the returned TResponse so you can locate and modify that block.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/pages/tools/ios-udid-finder/result.astro (1)
98-110:⚠️ Potential issue | 🟠 MajorHandle clipboard failures and announce copy status to assistive tech.
navigator.clipboard.writeTextis unguarded and can reject, and the “Copied” state is not announced via anaria-liveregion. Please wrap copy intry/catchand publish success/failure to a polite live region.As per coding guidelines, dynamic content must use
aria-liveregions for accessibility.
🧹 Nitpick comments (1)
src/lib/tools/signing.ts (1)
59-64: Consider usingreplaceAllfor consistency with modern JS APIs.The regex with the global flag works correctly, but
replaceAllis the preferred modern method per static analysis.Suggested change
function cleanField(value: unknown, maxLength = MAX_FIELD_LENGTH): string { return String(value ?? '') .trim() - .replace(/\s+/g, ' ') + .replaceAll(/\s+/g, ' ') .slice(0, maxLength) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/tools/signing.ts` around lines 59 - 64, The cleanField function uses String.prototype.replace with a global regex; update it to use String.prototype.replaceAll for consistency with modern JS APIs by replacing the .replace(/\s+/g, ' ') call with .replaceAll(/\s+/g, ' ') (keeping the same pattern and maxLength behavior) in the cleanField function referenced alongside MAX_FIELD_LENGTH to preserve trimming, collapsing whitespace, and slicing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/tools/signing.ts`:
- Around line 116-122: The validateValidityYears function currently uses value
?? '10' which still stringifies non-nullish non-primitive inputs and can produce
NaN; update validateValidityYears to explicitly handle null/undefined and
non-primitive types: if value == null or value === '' return 10, if typeof value
=== 'number' use Math.floor(value), if typeof value === 'string' trim and
parseInt, otherwise treat as unspecified and default to 10; then validate the
parsed integer is finite and between 1 and 35 (throwing the same error if not).
This keeps the function robust against objects/arrays while preserving the
10-year default and existing validation.
- Around line 259-263: The PKCS#12 creation call using forge.pkcs12.toPkcs12Asn1
currently uses the legacy '3des' algorithm; update that call (the pkcs12 =
forge.pkcs12.toPkcs12Asn1(...) invocation that passes keyPair.privateKey,
certificate, input.password and options including friendlyName: input.alias and
generateLocalKeyId: true) to use a modern cipher by replacing algorithm: '3des'
with algorithm: 'aes256' so the resulting PKCS#12 uses AES-256 encryption for
certificate storage.
In `@src/pages/tools/ios-udid-finder/result.astro`:
- Around line 2-4: The page is missing the required page metadata helpers and
keywords: update the Astro component to build and export a metadata object that
uses the JSON-LD helpers from src/lib/ldJson.ts (e.g., import and call the
appropriate helpers) and include a keywords array alongside title and
description; ensure the metadata uses getRelativeLocaleUrl for canonical or
locale URLs if needed and keep the existing decodePayload usage intact (look for
decodePayload and add metadata export near the top of the file so the page
follows the repo standards).
- Around line 8-13: This page reads UDID_RESULT_COOKIE and renders sensitive
data but doesn't disable caching; update the route to add a Cache-Control:
no-store response header for the page that reads decodePayload(cookieValue) (and
after you delete the cookie via Astro.cookies.delete(UDID_RESULT_COOKIE, { path:
UDID_RESULT_PATH })). Concretely, in the same component where
UDID_RESULT_COOKIE, UDID_RESULT_PATH, and decodePayload are used, set the HTTP
response header "Cache-Control" to "no-store" (using the framework's
response/headers API for Astro pages) so browsers and intermediaries will not
cache the UDID result.
---
Nitpick comments:
In `@src/lib/tools/signing.ts`:
- Around line 59-64: The cleanField function uses String.prototype.replace with
a global regex; update it to use String.prototype.replaceAll for consistency
with modern JS APIs by replacing the .replace(/\s+/g, ' ') call with
.replaceAll(/\s+/g, ' ') (keeping the same pattern and maxLength behavior) in
the cleanField function referenced alongside MAX_FIELD_LENGTH to preserve
trimming, collapsing whitespace, and slicing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d710593-f25a-4c03-93e0-05a7c1e2a1a2
📒 Files selected for processing (3)
src/lib/tools/signing.tssrc/pages/api/tools/ios-udid-finder/callback.tssrc/pages/tools/ios-udid-finder/result.astro
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/api/tools/ios-udid-finder/callback.ts
Comment on lines
+116
to
+122
| function validateValidityYears(value: unknown): number { | ||
| const parsed = Number.parseInt(String(value ?? '10'), 10) | ||
| if (!Number.isFinite(parsed) || parsed < 1 || parsed > 35) { | ||
| throw new Error('Validity must be between 1 and 35 years.') | ||
| } | ||
| return parsed | ||
| } |
Contributor
There was a problem hiding this comment.
Default value logic may not behave as intended for non-nullish invalid types.
The value ?? '10' pattern only applies the default when value is null or undefined. If an unexpected type (e.g., object) is passed, it will stringify to '[object Object]' and parseInt returns NaN, which is then caught by the validation. While the error is caught, the code intent seems to be defaulting to 10 years when unspecified—consider making this explicit.
Suggested fix
function validateValidityYears(value: unknown): number {
- const parsed = Number.parseInt(String(value ?? '10'), 10)
+ const raw = value == null ? '10' : String(value)
+ const parsed = Number.parseInt(raw, 10)
if (!Number.isFinite(parsed) || parsed < 1 || parsed > 35) {
throw new Error('Validity must be between 1 and 35 years.')
}
return parsed
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function validateValidityYears(value: unknown): number { | |
| const parsed = Number.parseInt(String(value ?? '10'), 10) | |
| if (!Number.isFinite(parsed) || parsed < 1 || parsed > 35) { | |
| throw new Error('Validity must be between 1 and 35 years.') | |
| } | |
| return parsed | |
| } | |
| function validateValidityYears(value: unknown): number { | |
| const raw = value == null ? '10' : String(value) | |
| const parsed = Number.parseInt(raw, 10) | |
| if (!Number.isFinite(parsed) || parsed < 1 || parsed > 35) { | |
| throw new Error('Validity must be between 1 and 35 years.') | |
| } | |
| return parsed | |
| } |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 117-117: 'value ?? '10'' will use Object's default stringification format ('[object Object]') when stringified.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/tools/signing.ts` around lines 116 - 122, The validateValidityYears
function currently uses value ?? '10' which still stringifies non-nullish
non-primitive inputs and can produce NaN; update validateValidityYears to
explicitly handle null/undefined and non-primitive types: if value == null or
value === '' return 10, if typeof value === 'number' use Math.floor(value), if
typeof value === 'string' trim and parseInt, otherwise treat as unspecified and
default to 10; then validate the parsed integer is finite and between 1 and 35
(throwing the same error if not). This keeps the function robust against
objects/arrays while preserving the 10-year default and existing validation.
Comment on lines
+259
to
+263
| const pkcs12 = forge.pkcs12.toPkcs12Asn1(keyPair.privateKey, certificate, input.password, { | ||
| algorithm: '3des', | ||
| friendlyName: input.alias, | ||
| generateLocalKeyId: true, | ||
| }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does node-forge support AES-256 encryption for PKCS#12 toPkcs12Asn1?
💡 Result:
Yes, node-forge supports AES-256 encryption for PKCS#12 via the toPkcs12Asn1 function. The function accepts an options object where the 'algorithm' parameter can be set to 'aes256' for private key encryption in the PKCS#12 structure. This is explicitly documented in TypeScript definitions and example code from technical sources, which list supported algorithms as '3des', 'aes128', 'aes192', or 'aes256'. Node-forge's cipher implementation supports AES-256 (32-byte keys), and PKCS#12 uses PBES2 with these ciphers. Official documentation recommends TripleDES for some compatibility but confirms AES support. Example usage: forge.pkcs12.toPkcs12Asn1(privateKey, certificate, password, {algorithm: 'aes256'}).
Citations:
- 1: https://tessl.io/registry/tessl/npm-node-forge/1.3.0/files/docs/pkcs.md
- 2: https://www.npmjs.com/package/node-forge
- 3: https://github.com/digitalbazaar/forge
- 4: https://npmtest.github.io/node-npmtest-node-forge/build/coverage.html/node-npmtest-node-forge/node-forge/lib/pkcs12.js.html
- 5: https://www.npmjs.com/package/node-forge/v/0.1.12
Replace weak 3DES with AES-256 for PKCS#12 encryption.
The '3des' algorithm is legacy cryptography. Node-forge's PKCS#12 implementation supports 'aes256' as a direct replacement. Change line 260 from algorithm: '3des' to algorithm: 'aes256' to use modern encryption for certificate storage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/tools/signing.ts` around lines 259 - 263, The PKCS#12 creation call
using forge.pkcs12.toPkcs12Asn1 currently uses the legacy '3des' algorithm;
update that call (the pkcs12 = forge.pkcs12.toPkcs12Asn1(...) invocation that
passes keyPair.privateKey, certificate, input.password and options including
friendlyName: input.alias and generateLocalKeyId: true) to use a modern cipher
by replacing algorithm: '3des' with algorithm: 'aes256' so the resulting PKCS#12
uses AES-256 encryption for certificate storage.
Comment on lines
+2
to
+4
| import Layout from '@/layouts/Layout.astro' | ||
| import { decodePayload } from '@/lib/tools/udid' | ||
| import { getRelativeLocaleUrl } from 'astro:i18n' |
Contributor
There was a problem hiding this comment.
Add required page metadata helpers for new src/pages entry.
The page defines title/description, but it does not use src/lib/ldJson.ts helpers or include keywords in the metadata object, which is required by the repo page standards for new Astro pages.
As per coding guidelines, src/pages/**/*.{astro,ts,tsx,js,jsx} must include JSON-LD structured data using src/lib/ldJson.ts helpers, and src/pages/**/*.astro should use a keywords prop in page metadata.
Also applies to: 20-24
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/tools/ios-udid-finder/result.astro` around lines 2 - 4, The page is
missing the required page metadata helpers and keywords: update the Astro
component to build and export a metadata object that uses the JSON-LD helpers
from src/lib/ldJson.ts (e.g., import and call the appropriate helpers) and
include a keywords array alongside title and description; ensure the metadata
uses getRelativeLocaleUrl for canonical or locale URLs if needed and keep the
existing decodePayload usage intact (look for decodePayload and add metadata
export near the top of the file so the page follows the repo standards).
Comment on lines
+8
to
+13
| const cookieValue = Astro.cookies.get(UDID_RESULT_COOKIE)?.value ?? null | ||
| const payload = decodePayload(cookieValue) | ||
|
|
||
| if (cookieValue) { | ||
| Astro.cookies.delete(UDID_RESULT_COOKIE, { path: UDID_RESULT_PATH }) | ||
| } |
Contributor
There was a problem hiding this comment.
Prevent caching of UDID result responses.
This page renders sensitive device identifiers but does not explicitly disable caching on the response. Add Cache-Control: no-store to avoid browser/intermediary reuse of another user’s payload.
🔐 Proposed fix
const cookieValue = Astro.cookies.get(UDID_RESULT_COOKIE)?.value ?? null
const payload = decodePayload(cookieValue)
+Astro.response.headers.set('Cache-Control', 'no-store, max-age=0, must-revalidate')
+Astro.response.headers.set('Pragma', 'no-cache')
+
if (cookieValue) {
Astro.cookies.delete(UDID_RESULT_COOKIE, { path: UDID_RESULT_PATH })
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const cookieValue = Astro.cookies.get(UDID_RESULT_COOKIE)?.value ?? null | |
| const payload = decodePayload(cookieValue) | |
| if (cookieValue) { | |
| Astro.cookies.delete(UDID_RESULT_COOKIE, { path: UDID_RESULT_PATH }) | |
| } | |
| const cookieValue = Astro.cookies.get(UDID_RESULT_COOKIE)?.value ?? null | |
| const payload = decodePayload(cookieValue) | |
| Astro.response.headers.set('Cache-Control', 'no-store, max-age=0, must-revalidate') | |
| Astro.response.headers.set('Pragma', 'no-cache') | |
| if (cookieValue) { | |
| Astro.cookies.delete(UDID_RESULT_COOKIE, { path: UDID_RESULT_PATH }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/tools/ios-udid-finder/result.astro` around lines 8 - 13, This page
reads UDID_RESULT_COOKIE and renders sensitive data but doesn't disable caching;
update the route to add a Cache-Control: no-store response header for the page
that reads decodePayload(cookieValue) (and after you delete the cookie via
Astro.cookies.delete(UDID_RESULT_COOKIE, { path: UDID_RESULT_PATH })).
Concretely, in the same component where UDID_RESULT_COOKIE, UDID_RESULT_PATH,
and decodePayload are used, set the HTTP response header "Cache-Control" to
"no-store" (using the framework's response/headers API for Astro pages) so
browsers and intermediaries will not cache the UDID result.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
This adds a new
/tools/hub plus three new mobile-signing utility pages:tools/ios-certificate-generator/tools/ios-udid-finder/tools/android-keystore-generator/Each page is written to capture high-intent search traffic around signing setup and device onboarding, with long-form explanatory copy, FAQ content, internal links, and structured data.
Product impact
mobileconfigsupport.Implementation notes
PUBLIC_IOS_UDID_CERTIFICATE_LINKso the certificate download URL can be configured without code changes.src/config/plugins.tswas cleaned up to remove Astro component imports from a plain TypeScript config file, which was blocking the build earlier in the pipeline.Validation
bunx prettier --write src/lib/tools/signing.ts src/lib/tools/udid.ts src/pages/api/tools/ios-udid-finder/profile.tsbun run buildcurrently fails locally in the Cloudflare Vite export-types helper withMissing field \moduleType`` before user route code runs. I traced this to the plugin's virtual export-types import path rather than the new tool routes, so CI on GitHub is the authoritative check for this branch.Summary by CodeRabbit
New Features
Documentation
Navigation
Chores