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

feat(image): upload image endpoint #300

Closed

Conversation

hassnian
Copy link
Contributor

@hassnian hassnian commented Jun 10, 2024

Context

Add /image/upload endpoint

Ref

PR Type

  • Bugfix
  • Feature
  • Refactoring
  • Chore

@hassnian hassnian requested a review from a team as a code owner June 10, 2024 13:01
@hassnian hassnian requested review from yangwao, preschian and vikiival and removed request for a team and yangwao June 10, 2024 13:01
Copy link

socket-security bot commented Jun 10, 2024

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@kodadot/workers-utils@1.0.0 None 0 0 B
npm/hono@3.12.12 None 0 566 kB yusukebe
npm/hono@4.3.6 None 0 815 kB yusukebe
npm/hono@4.4.5 None 0 887 kB yusukebe
npm/nuxt-og-image@3.0.0-rc.53 Transitive: environment, network +1 9.89 MB harlan_zw
npm/prettier@3.2.5 environment, filesystem, unsafe 0 8.39 MB prettier-bot
npm/prettier@3.3.2 environment, filesystem, unsafe 0 8.25 MB prettier-bot
npm/remark-parse@11.0.0 None 0 19.5 kB wooorm
npm/remark-stringify@11.0.0 None 0 19.6 kB wooorm
npm/typescript@5.4.5 None 0 32.4 MB typescript-bot
npm/ufo@1.5.3 None 0 103 kB pi0
npm/unified@11.0.4 None 0 147 kB wooorm
npm/vfile-matter@5.0.0 None 0 13.5 kB wooorm

View full report↗︎

Copy link
Member

@vikiival vikiival left a comment

Choose a reason for hiding this comment

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

Use ipfs paths,
chceck just-ipfs-cid package

@hassnian
Copy link
Contributor Author

Use ipfs paths, chceck just-ipfs-cid package

can't find any reference to that package, can I get a link ?

@vikiival
Copy link
Member

@hassnian hassnian requested a review from vikiival June 11, 2024 12:57
@vikiival
Copy link
Member

cc @preschian as he coded 90% of the worker

app.post('/upload', vValidator('form', uploadImageRequestSchema), async (c) => {
const { file } = await c.req.parseBody<UploadImage>()

const path = await Hash.of(new Uint8Array(await file.arrayBuffer()))
Copy link
Member

Choose a reason for hiding this comment

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

nit: actually I'm good with crypto.randomUUID(). this package seems old

Copy link
Member

Choose a reason for hiding this comment

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

Wont it break the schema of cfi?

Copy link
Member

Choose a reason for hiding this comment

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

Since this endpoint is for general images to put on Cloudflare Images, and we store the URL on the profile database, it would not conflict

Copy link
Member

Choose a reason for hiding this comment

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

so isn't it easier to use user's address?

Copy link
Member

Choose a reason for hiding this comment

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

This means it always overrides the image after every upload. I'm worried about the security side. Is it possible that another user changed our profile image?

Copy link
Member

Choose a reason for hiding this comment

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

So let's make this PR ${address}_image and ${adress}_banner then let's do opsec in followup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

How about this one? I prefer to put the endpoint to profile-worker

Anyway, using the address format is closely coupled with the profile. I think putting the endpoint in the profile-worker is better

And the address is not verified by this worker. I can hijack it, tho. I can show it to you guys if you want

Copy link
Contributor Author

@hassnian hassnian Jun 20, 2024

Choose a reason for hiding this comment

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

And the address is not verified by this worker. I can hijack it, tho. I can show it to you guys if you want

yep , you can change the image of other users without updating their profile

maybe I misunderstood it but didn't do this bcz of @vikiival comment

How about this one? I prefer to put the endpoint to profile-worker

+1 , will wait for https://github.com/kodadot/private-workers/pull/182 if we want to have the verifier

Copy link
Member

Choose a reason for hiding this comment

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

Let's close this one and create a new endpoint for the profile worker instead. If we go with this one, the attacker can update some user profiles without verifying the signature

services/image/src/routes/image.ts Show resolved Hide resolved
@vikiival
Copy link
Member

@preschian pls merge if its ok

@hassnian hassnian requested a review from Jarsen136 as a code owner June 20, 2024 07:01
@hassnian hassnian removed the request for review from Jarsen136 June 20, 2024 07:02
@hassnian
Copy link
Contributor Author

@preschian any idea why that test is failing ? shouldn't affect that endpoint.

CleanShot 2024-06-20 at 12 31 00@2x

@preschian
Copy link
Member

any idea why that test is failing ? shouldn't affect that endpoint.

yes, safe to ignore. because there's some change in this PR #308. the test file is hit real endpoint instead of local

@vikiival
Copy link
Member

@preschian pls merge if its ok

Cc @preschian

@vikiival
Copy link
Member

@hassnian pls resolve conflicts

@preschian
Copy link
Member

I close this one. Let's create the endpoint on the profile-worker instead. If we merge this, the attacker can change the other profile by hitting this endpoint directly

@preschian preschian closed this Jun 24, 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.

3 participants