Skip to content

🧹 [Code Health] Fix swallowed errors in validators.ts catch blocks #349

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
is0692vs wants to merge 2 commits into
base: main
Choose a base branch
from
+15 −3
Open

🧹 [Code Health] Fix swallowed errors in validators.ts catch blocks #349

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4b2c355
🧹 [Code Health] Fix swallowed errors in validators.ts catch blocks
google-labs-jules[bot] Jun 6, 2026
ef9a11e
🧹 [Code Health] Fix swallowed errors in validators.ts catch blocks
google-labs-jules[bot] Jun 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/lib/__tests__/validators.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,4 +206,14 @@ describe("isTrustedFontUrl", () => {
isTrustedFontUrl("https://localhost:3000/fonts/NotoSans-Regular.ttf", "https://localhost:3000"),
).toBe(false);
});
it("handles invalid APP_URL gracefully and falls back to allowlist", () => {
process.env.APP_URL = "invalid-url";
expect(
isTrustedFontUrl(
"https://github-user-summary.vercel.app/fonts/NotoSans-Regular.ttf",
"https://github-user-summary.vercel.app"
)
).toBe(true);
});

});
8 changes: 5 additions & 3 deletions src/lib/validators.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { logger } from "./logger";
/**
* Validates a GitHub username.
Comment on lines +1 to 3

@greptile-apps greptile-apps Bot Jun 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 インポート文の直後に空行がありません。TypeScript の一般的なスタイルガイドでは、インポートブロックと最初のコードブロック（JSDoc コメントを含む）の間に空行を入れることが推奨されています。

Suggested change
import { logger } from "./logger";
/**
* Validates a GitHub username.
import { logger } from "./logger";
/**
* Validates a GitHub username.
Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/lib/validators.ts
Line: 1-3

Comment:
インポート文の直後に空行がありません。TypeScript の一般的なスタイルガイドでは、インポートブロックと最初のコードブロック（JSDoc コメントを含む）の間に空行を入れることが推奨されています。

```suggestion
import { logger } from "./logger";

/**
 * Validates a GitHub username.
```

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

* Rules:
Expand Down Expand Up @@ -64,8 +65,8 @@ function getTrustedFontOrigins(): Set<string> {
if (configuredOrigin.startsWith("https://")) {
origins.add(configuredOrigin);
}
} catch {
// Ignore invalid deployment configuration and fall back to the fixed allowlist.
} catch (err) {
logger.warn("Invalid APP_URL deployment configuration. Falling back to fixed allowlist.", err);
}
}

Expand Down Expand Up @@ -121,7 +122,8 @@ export function isTrustedFontUrl(url: string, allowedOrigin?: string): boolean {
}

return false;
} catch {
} catch (err) {
logger.warn("Invalid URL provided to isTrustedFontUrl.", err);
return false;
}
Comment on lines +125 to 128

@gemini-code-assist gemini-code-assist Bot Jun 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Logging a warning for every invalid URL passed to isTrustedFontUrl can lead to log pollution and potential log-flooding vulnerabilities if the input is user-controlled. Since new URL(url) throwing an error is the standard way to detect invalid URLs in JavaScript/TypeScript, an invalid URL is an expected validation failure rather than an exceptional system warning.

It is recommended to revert this change for isTrustedFontUrl and keep the silent catch block, as returning false is the correct and fully handled outcome for invalid inputs.

  } catch {
    return false;
  }

Comment on lines +125 to 128

@greptile-apps greptile-apps Bot Jun 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 isTrustedFontUrl の catch ブロックは new URL(url)（101行目）と new URL(allowedOrigin)（115行目）の両方で発生した例外を捕捉します。現在のログメッセージ "Invalid URL provided to isTrustedFontUrl." は url が不正であるかのように読めますが、allowedOrigin が不正な場合も同じメッセージが出力されます。どちらのパラメータが原因かをデバッグ時に把握しやすくするため、より具体的なメッセージにすると良いでしょう。

Suggested change
} catch (err) {
logger.warn("Invalid URL provided to isTrustedFontUrl.", err);
return false;
}
} catch (err) {
logger.warn("Invalid URL provided to isTrustedFontUrl (url or allowedOrigin is malformed).", err);
return false;
}
Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/lib/validators.ts
Line: 125-128

Comment:
`isTrustedFontUrl` の catch ブロックは `new URL(url)`（101行目）と `new URL(allowedOrigin)`（115行目）の両方で発生した例外を捕捉します。現在のログメッセージ `"Invalid URL provided to isTrustedFontUrl."` は `url` が不正であるかのように読めますが、`allowedOrigin` が不正な場合も同じメッセージが出力されます。どちらのパラメータが原因かをデバッグ時に把握しやすくするため、より具体的なメッセージにすると良いでしょう。

```suggestion
  } catch (err) {
    logger.warn("Invalid URL provided to isTrustedFontUrl (url or allowedOrigin is malformed).", err);
    return false;
  }
```

How can I resolve this? If you propose a fix, please make it concise.

}
Loading