Skip to content
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.

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

[Bugfix] Fix CORS SSRF security issue #4285

Merged
merged 7 commits into from
Mar 13, 2024

Conversation

fred-bf
Copy link
Contributor

@fred-bf fred-bf commented Mar 13, 2024

Fix CORS SSRF security issue reported by: #4283

  • Migrate Upstash API
  • Migrate WebDAV API
  • Remove request to CORS api

Copy link

vercel bot commented Mar 13, 2024

@fred-bf is attempting to deploy a commit to the NextChat Team on Vercel.

A member of the Team first needs to authorize it.

@fred-bf fred-bf requested a review from Yidadaa March 13, 2024 16:02
@fred-bf fred-bf self-assigned this Mar 13, 2024
Copy link
Contributor

Your build has completed!

Preview deployment

@H0llyW00dzZ
Copy link
Contributor

by the way, for the webdav its difficult to filter

@fred-bf
Copy link
Contributor Author

fred-bf commented Mar 13, 2024

by the way, for the webdav its difficult to filter

My current idea is to filter requests by folder and fileName

@fred-bf
Copy link
Contributor Author

fred-bf commented Mar 13, 2024

the folder name cannot be changed by user right now

@H0llyW00dzZ
Copy link
Contributor

by the way, for the webdav its difficult to filter

My current idea is to filter requests by folder and fileName

There's another approach beyond using folder/fileName.

This approach involves:

Verifying methods for requests. Then, in this one, apply it exclusively only for syncing WebDAV, Upstah, or others syncing that for implementing in future.

Ref:

@fred-bf
Copy link
Contributor Author

fred-bf commented Mar 13, 2024

by the way, for the webdav its difficult to filter

My current idea is to filter requests by folder and fileName

There's another approach beyond using folder/fileName.

This approach involves:

Verifying methods for requests. Then, in this one, apply it exclusively only for syncing WebDAV, Upstah, or others syncing that for implementing in future.

Ref:

Yes, it is indeed easier to maintain from a development perspective. I'm worried that the loopholes here may be overlooked in the future if we put them all together. It would be more intuitive for each specific interface to maintain the forwarding capability separately.

@H0llyW00dzZ
Copy link
Contributor

by the way, for the webdav its difficult to filter

My current idea is to filter requests by folder and fileName

There's another approach beyond using folder/fileName.
This approach involves:
Verifying methods for requests. Then, in this one, apply it exclusively only for syncing WebDAV, Upstah, or others syncing that for implementing in future.
Ref:

Yes, it is indeed easier to maintain from a development perspective. I'm worried that the loopholes here may be overlooked in the future if we put them all together. It would be more intuitive for each specific interface to maintain the forwarding capability separately.

No need to worry, it's possible to modularize it separately, like this example:

image

That method switch golang

@fred-bf fred-bf requested a review from jokester March 13, 2024 17:25
Copy link

vercel bot commented Mar 13, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nextchat ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 13, 2024 6:22pm

Copy link

@jokester jokester left a comment

Choose a reason for hiding this comment

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

lgtm!!!

This should make it much harder to do bad things with upstash / webdav APIs.

for app/api/cors/[...path]/route.ts I suppose some fix is still needed.

@Yidadaa
Copy link
Collaborator

Yidadaa commented Mar 13, 2024

Let's turn it into a RPC, here is a proposal:

type WebDavConfig = {
  type: 'webdav',
  endpoint: string,
  username: string,
  password: string
}

type UpstashConfig = {
  type: 'upstash',
  endpoint: string,
  username: STORAGE_KEY,
  apiKey: string
}

type SyncAction = {
  type: 'check'
} | {
  type: 'get',
  key: string,
} | {
  type: 'set',
  key: string,
  value: string
}

type SyncPayload = {
  config: WebDavConfig | UpstashConfig,
  action: SyncAction
}

// client side
function createUpstashClient() {
  return async check() {
    return await call('/api/sync', { /* webdav config */ })
  },
  // get(key), set(key, value)
}

// server side, /api/sync/route.ts
async function handle(req) {
  const payload = JSON.parse(await req.json()) as SyncPayload;

  // dispatch payload to server side check/get/set actions
}

The main idea is to move all the sync actions that were previously done on the client side to the server side. The downside is that the desktop app can no longer use the sync feature unless we keep the official sync endpoint built in.

@jokester
Copy link

move all the sync actions that were previously done on the client side to the server side

Does that mean /cors API can be removed ? Because I don't see its other uses if browser can do sync without CORS-capable storage backends.

@fred-bf
Copy link
Contributor Author

fred-bf commented Mar 13, 2024

fix: remove corsFetch

cors api already removed in commit eebc334

@fred-bf
Copy link
Contributor Author

fred-bf commented Mar 13, 2024

@Yidadaa for native client we can use tauri's http client to perform request without CORS issue. but it may take a while to refactor the code. My idea is that we release a patch version first, and then migrate the whole thing to the new interface.

@H0llyW00dzZ
Copy link
Contributor

Let's turn it into a RPC, here is a proposal:

type WebDavConfig = {
  type: 'webdav',
  endpoint: string,
  username: string,
  password: string
}

type UpstashConfig = {
  type: 'upstash',
  endpoint: string,
  username: STORAGE_KEY,
  apiKey: string
}

type SyncAction = {
  type: 'check'
} | {
  type: 'get',
  key: string,
} | {
  type: 'set',
  key: string,
  value: string
}

type SyncPayload = {
  config: WebDavConfig | UpstashConfig,
  action: SyncAction
}

// client side
function createUpstashClient() {
  return async check() {
    return await call('/api/sync', { /* webdav config */ })
  },
  // get(key), set(key, value)
}

// server side, /api/sync/route.ts
async function handle(req) {
  const payload = JSON.parse(await req.json()) as SyncPayload;

  // dispatch payload to server side check/get/set actions
}

The main idea is to move all the sync actions that were previously done on the client side to the server side. The downside is that the desktop app can no longer use the sync feature unless we keep the official sync endpoint built in.

It would be better to keep the official sync that can be used in the desktop app because it is more secure and safe (essentially built-in and isolated) in the desktop environment.

@fred-bf fred-bf linked an issue Mar 13, 2024 that may be closed by this pull request
@fred-bf fred-bf marked this pull request as ready for review March 13, 2024 18:21
@fred-bf
Copy link
Contributor Author

fred-bf commented Mar 13, 2024

Merge the current PR first, and if there is any other questions, you could comment here or file a new issue

@fred-bf fred-bf merged commit a22141c into ChatGPTNextWeb:main Mar 13, 2024
3 checks passed
gaogao1030 pushed a commit to gaogao1030/ChatGPT-Next-Web that referenced this pull request May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] NextChat cors SSRF 漏洞(CVE-2023-49785)
4 participants