fix: stop exposing client-side tokens#1329
fix: stop exposing client-side tokens#1329Abhash-Chakraborty wants to merge 5 commits intorecodehive:mainfrom
Conversation
|
@Abhash-Chakraborty is attempting to deploy a commit to the recode Team on Vercel. A member of the Team first needs to authorize it. |
|
Thank you for submitting your pull request! 🙌 We'll review it as soon as possible. The estimated time for response is 5–8 hrs. In the meantime, please provide all necessary screenshots and make sure you run - npm build run , command and provide a screenshot, a video recording, or an image of the update you made below, which helps speed up the review and assignment. If you have questions, reach out to LinkedIn. Your contributions are highly appreciated!😊 Note: I maintain the repo issue every day twice at 8:00 AM IST and 9:00 PM IST. If your PR goes stale for more than one day, you can tag and comment on this same issue by tagging @sanjay-kv. We are here to help you on this journey of open source. Consistent 20 contributions are eligible for sponsorship 💰 🎁 check our list of amazing people we sponsored so far: GitHub Sponsorship. ✨ 📚Your perks for contribution to this community 👇🏻
If there are any specific instructions or feedback regarding your PR, we'll provide them here. Thanks again for your contribution! 😊 |
|
@sanjay-kv Can you please review this? |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR removes previously client-exposed secrets from the Docusaurus site configuration and updates docs/code to avoid making authenticated GitHub requests from browser-bundled code.
Changes:
- Remove GitHub token plumbing from client config and client-side request headers.
- Remove hardcoded Shopify Storefront token fallback from the Docusaurus config.
- Update internal documentation to clarify tokens must remain server-side.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| wiki/Documentation.md | Updates guidance to avoid storing GitHub tokens in client-bundled Docusaurus config. |
| src/services/githubService.ts | Removes browser token injection from GitHub request headers. |
| src/lib/statsProvider.tsx | Stops reading GitHub token from Docusaurus context and removes Authorization header usage. |
| docusaurus.config.ts | Removes GitHub token exposure and deletes hardcoded Shopify token fallback. |
Comments suppressed due to low confidence (1)
wiki/Documentation.md:581
- The TypeScript code fence opened for the headers example isn’t closed before the next markdown heading (
#### Getting a Token:), which will cause the rest of this section to render as code. Add a closing ``` after the headers example.
Authenticated requests should be made from a server-side endpoint or serverless function so the token is never shipped to the browser:
```typescript
const headers: Record<string, string> = {
Authorization: `token ${YOUR_GITHUB_TOKEN}`,
Accept: "application/vnd.github.v3+json",
};
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Get headers for GitHub API requests | ||
| private getHeaders(): Record<string, string> { | ||
| const headers: Record<string, string> = { | ||
| return { | ||
| Accept: "application/vnd.github.v3+json", | ||
| "Content-Type": "application/json", | ||
| }; | ||
|
|
||
| // Add GitHub token if available in environment | ||
| // Note: In production, you might want to use a server-side proxy to avoid exposing tokens | ||
| if (typeof window !== "undefined" && (window as any).GITHUB_TOKEN) { | ||
| headers["Authorization"] = `token ${(window as any).GITHUB_TOKEN}`; | ||
| } | ||
|
|
||
| return headers; | ||
| } |
There was a problem hiding this comment.
The getHeaders() change removes Authorization, but this service makes requests to https://api.github.com/graphql (e.g., discussions count / discussions list). GitHub’s GraphQL API requires authentication, so these calls will now consistently fail (401) and the code will fall back to 0 discussions / mock discussions. Consider moving GraphQL calls behind a server-side endpoint (preferred), or switch to unauthenticated REST endpoints, or gate/disable these GraphQL features when no server-side auth is available.
|
@Adez017 could you check this? |
Sure I'll look into this |
|
Hi @Abhash-Chakraborty ,good fix on the intent — tokens absolutely should not be in customFields. But before this merges, please:
CC: @sanjay-kv |
| // Get headers for GitHub API requests | ||
| private getHeaders(): Record<string, string> { | ||
| const headers: Record<string, string> = { | ||
| return { | ||
| Accept: "application/vnd.github.v3+json", | ||
| "Content-Type": "application/json", | ||
| }; | ||
|
|
||
| // Add GitHub token if available in environment | ||
| // Note: In production, you might want to use a server-side proxy to avoid exposing tokens | ||
| if (typeof window !== "undefined" && (window as any).GITHUB_TOKEN) { | ||
| headers["Authorization"] = `token ${(window as any).GITHUB_TOKEN}`; | ||
| } | ||
|
|
||
| return headers; | ||
| } |
|
@Adez017 Check now |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
wiki/Documentation.md:588
- The
```typescriptcode block started here isn’t closed before the next heading (#### Getting a Token:). This will cause the rest of the document to render as code in Markdown. Add a closing triple-backtick after the headers example (after the};).
Authenticated requests should be made from a server-side endpoint or serverless function so the token is never shipped to the browser:
```typescript
const headers: Record<string, string> = {
Authorization: `token ${YOUR_GITHUB_TOKEN}`,
Accept: "application/vnd.github.v3+json",
};
#### Getting a Token:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // GitHub GraphQL requires authentication, so the browser should not call it directly. | ||
| private async getDiscussionsCount( | ||
| signal?: AbortSignal, | ||
| repoName: string = "Support", | ||
| ): Promise<number> { | ||
| if (!this.canUseGitHubGraphQL()) { | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
getDiscussionsCount() returns 0 when GraphQL is unavailable in the browser. That makes downstream UI/reporting treat the value as a real count ("0 discussions") rather than "unavailable", which is misleading. Consider representing this as null/undefined (and updating GitHubOrgStats + UI), or surfacing an explicit "unavailable" flag/message instead of overloading 0.
| if (!this.canUseGitHubGraphQL()) { | ||
| throw new Error(this.DISCUSSIONS_UNAVAILABLE_MESSAGE); | ||
| } | ||
|
|
There was a problem hiding this comment.
Even when canUseGitHubGraphQL() is true, fetchDiscussions() still calls https://api.github.com/graphql directly without any auth header. If/when this is invoked in a server-side context, it will still fail with 401 unless you route through an authenticated proxy or add server-side auth (e.g., from env). Consider failing fast with a clear configuration error when no server-side token/proxy is available, rather than attempting the call.
|
@copilot apply changes based on the comments in this thread |
|
Hi @Abhash-Chakraborty , i had made a look onto this one . there are some bugs still needs to be fixed . i would request you to look into this and try to resolve . CC: @sanjay-kv |
|
@Adez017 please check. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
src/lib/statsProvider.tsx:440
- With the
Authorizationheader removed, this provider now makes a large number of unauthenticated GitHub REST calls (org repos + up to 10 pages of PRs per repo, batched concurrently). GitHub’s anonymous rate limit is very low, so this will likely fail for most users with 403 rate-limit errors. Consider moving this data aggregation behind a server-side endpoint/proxy (with caching) or drastically reducing request volume (e.g., precomputed stats, fewer repos/pages, or build-time generation).
try {
const headers: Record<string, string> = {
Accept: "application/vnd.github.v3+json",
};
// Fetch both org stats and repos in parallel
const [orgStats, repos] = await Promise.all([
githubService.fetchOrganizationStats(signal),
fetchAllOrgRepos(headers),
wiki/Documentation.md:586
- The
typescriptcode fence opened for theheadersexample is never closed, so the rest of this section will render as code. Add a closing ``` after theheaderssnippet (after the `};`).
Authenticated requests should be made from a server-side endpoint or serverless function so the token is never shipped to the browser:
```typescript
const headers: Record<string, string> = {
Authorization: `token ${YOUR_GITHUB_TOKEN}`,
Accept: "application/vnd.github.v3+json",
};
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| markdown: { | ||
| mermaid: true, | ||
| hooks: { | ||
| onBrokenMarkdownLinks: "warn", | ||
| }, | ||
| }, |
There was a problem hiding this comment.
The new markdown.hooks.onBrokenMarkdownLinks config is likely not a recognized Docusaurus option (Docusaurus 3.x supports onBrokenMarkdownLinks as a top-level config). As written, this may be ignored and broken markdown links will use the default behavior instead of warn. Consider moving this to onBrokenMarkdownLinks: "warn" at the top level and removing the markdown.hooks nesting.
| private readonly CACHE_DURATION = 30 * 60 * 1000; // 30 minutes in milliseconds | ||
| private readonly BASE_URL = "https://api.github.com"; | ||
| private readonly DISCUSSIONS_UNAVAILABLE_MESSAGE = | ||
| "GitHub Discussions are disabled until a server-side GitHub proxy is configured."; |
There was a problem hiding this comment.
This error message is thrown both when running in the browser (GraphQL disabled) and when no server-side token is configured. The current wording implies a proxy is required, but the code also supports direct server-side calls via process.env.GITHUB_TOKEN; consider updating the message to reflect the actual requirements (e.g., “available only server-side; configure GITHUB_TOKEN or a proxy”).
| "GitHub Discussions are disabled until a server-side GitHub proxy is configured."; | |
| "GitHub Discussions are available only server-side; configure GITHUB_TOKEN or a server-side GitHub proxy."; |
There was a problem hiding this comment.
@Abhash-Chakraborty , I had a quick question for you . is this gonna work around the discussions also , ATM the discussion in prod are working fine .
|
Hi @Abhash-Chakraborty any updates on this one |
Adez017
added
the
don't-merge
Keep discussions live in production without exposing a client token.\n\nRoute discussion reads through a server-side API endpoint, preserve\nlegacy env compatibility during migration, and document the required\nenvironment variables.
|
@Adez017 Yes, this now covers the discussions flow as well, and they remain dynamic in prod.
For compatibility, I also kept support for the existing |
Adez017
left a comment
Adez017
left a comment
There was a problem hiding this comment.
@sanjay-kv , can you test the changes in vercel preview ? also try to validate everything .
|
@sanjay-kv Please update variables pre-deployment. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const [, repositories] = await Promise.all([ | ||
| this.fetchOrganizationInfo(signal), | ||
| this.fetchAllRepositories(signal), | ||
| ]); |
There was a problem hiding this comment.
fetchOrganizationStats() still performs fetchOrganizationInfo() (the first Promise in the Promise.all), but the result is intentionally ignored. This adds an extra GitHub API request on every refresh/cache miss, increasing latency and rate-limit risk. Consider removing the org-info call entirely (or using its data in the computed stats) so you only request what you need.
| const [, repositories] = await Promise.all([ | |
| this.fetchOrganizationInfo(signal), | |
| this.fetchAllRepositories(signal), | |
| ]); | |
| const repositories = await this.fetchAllRepositories(signal); |
| const payload = (await response.json()) as GitHubDiscussionsResponse; | ||
|
|
||
| const data = await response.json(); | ||
| if (!response.ok) { | ||
| throw new Error(payload.message || "Failed to fetch GitHub discussions."); | ||
| } |
There was a problem hiding this comment.
fetchDiscussions() unconditionally calls response.json() before checking response.ok. If the /api/github-discussions route is missing/misconfigured (404 HTML) or returns a non-JSON error body, this will throw a JSON parse error and lose the real HTTP status/context. Consider parsing defensively (e.g., read text first, or only JSON-parse when the Content-Type is JSON) and include the status code in the thrown error when available.
| export default async function handler(req, res) { | ||
| const token = getToken(); |
There was a problem hiding this comment.
This new API handler uses ESM syntax (export default) in a .js file, but package.json does not declare "type": "module". In a standard Node.js/CommonJS runtime this will fail to load with a syntax error. Consider switching this to CommonJS exports (module.exports = handler) or renaming to .mjs / configuring the project/runtime to treat api/* as ESM consistently.
| res.setHeader( | ||
| "Cache-Control", | ||
| "public, s-maxage=300, stale-while-revalidate=600", | ||
| ); |
There was a problem hiding this comment.
Cache-Control is set before checking whether a token is configured and before the GitHub fetch succeeds. As written, 503 (no token) and 502 (upstream failure) responses will also be cached by CDNs for 5 minutes (s-maxage=300), potentially prolonging outages after configuration fixes. Consider only setting long-lived caching on successful (200) responses, and using a much shorter TTL or no-store for error responses.
| Go to GitHub Settings → Developer settings → Personal access tokens → Tokens (classic) | ||
| Generate new token | ||
| Select scopes: public_repo, read:org | ||
| Copy the token (you won't see it again!) |
There was a problem hiding this comment.
The “Select scopes” guidance here omits read:discussion, but .env.example now recommends public_repo, read:org, read:discussion for the token used by /api/github-discussions. Please align these docs so users generate a token with the scopes actually required for the GraphQL discussions query.
|
@Abhash-Chakraborty , some of the suggestion from copilot are need to be take care . try to resolve them. |
Summary
Testing