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

Add avatar photo upload component #101

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

makoncline
Copy link
Collaborator

User can upload avatar image
File is checked for type and size
File is uploaded to AWS S3 bucket using pre signed url
File is deleted with pre signed url
NEXT API route used for pre signing urls
Bucket has public read access for viewing

not many changes needed to allow upload of multiple user photos.
(don't limit the file list to a single value and keep displaying upload button)

There are MANY things that can be improved, but it's what i've got.
image
image
image

@benjie
Copy link
Member

benjie commented Jan 24, 2020

Thanks for this! It's going to take me a while to getting around to improving it/merging it, but in the mean time it acts as a great demonstration of how to accomplish this to other users 👍

You need to add @app/graphql as a dependency to @app/components/package.json; I believe this is why CI is failing.

@benjie
Copy link
Member

benjie commented Jan 27, 2020

$ prettier --ignore-path .eslintignore --check '**/*.{js,jsx,ts,tsx,graphql,md}'
Checking formatting...
README.md
Code style issues found in the above file(s). Forgot to run Prettier?

^ To solve this, yarn lint:fix (yes, we even format markdown! 😁)

Copy link
Collaborator

@singingwolfboy singingwolfboy left a comment

Choose a reason for hiding this comment

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

This is a great start, but I found a few things that should probably be improved before merge. In particular, I think this is passing the AWS secret key to the browser, which is a big security problem.

if (err) {
res.json({ success: false, err });
} else {
res.json({ success: true, url });
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than using a success key in the JSON payload, it's better to use the HTTP status code to communicate an error. In this case, if the error is coming from S3, that indicates the status "502 Bad Gateway" because our server is acting as a gateway (or proxy) to S3.

if (err) {
  res.status(504).json({ err });
} else {
  res.json({ url });
}

} from "@app/graphql";
import { ApolloError } from "apollo-client";

export function slugify(string: string) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of defining your own function to make a slug, it's probably better to use an existing package. I just searched through yarn.lock, and it looks like we already depends on a library called unique-slug, which would probably suit your needs just fine.

};

const uploadButton = (
<div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

For accessibility purposes, this should use a <button> element. It should probably also have aria-label set to something like "Upload avatar". You can use the Button component in antd.

serverRuntimeConfig: {
BUCKET: BUCKET,
AWSACCESSKEYID: AWSACCESSKEYID,
AWSSECRETKEY: AWSSECRETKEY,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is extremely insecure: you are passing the AWS secret key to the browser! That means any user of your application can find your secret key, and make AWS API requests on your behalf! 😱

A better, more secure way of doing this is to generate a signed upload URL on the server, and only pass that signed URL to the client. That way, the client has enough information to do what it needs to do (upload files to S3), but nothing more than that. Fortunately, there is a library called react-s3-uploader that is designed to work in exactly this way. I highly suggest using this library.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the code review! I've been programming solo so far, so its good to get feedback.

This is the suggested way to pass server only runtime configuration to Next.

image

I think..... that these are only available on the server side, since it's used only in a next api route.

The intent is that this does exactly what you said, "...generate a signed upload URL on the server, and only pass that signed URL to the client".
I'll look into using the react-s3-uploader library.

Can you confirm if this is actually sending the serverRuntimeConfig: credentials to the client??
If so, I have some other things I need to change quickly 😳...
At least I'm using limited IAM roles.

Copy link
Collaborator

@singingwolfboy singingwolfboy Feb 1, 2020

Choose a reason for hiding this comment

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

Oh dear, I'm sorry for the panic. After reading the Next.js docs more carefully, I think you're right: you've done it correctly, and the credentials should stay on the server.

I saw that you put the credentials in the config, and then imported and used them in a file under the /pages directory -- and in my limited experience with Next.js, I thought that everything under /pages is run on both the server and the client. But I didn't know that serverRuntimeConfig is special and /pages/api is special. (Seems weird to me that an API is a subset of a page...)

I tried to run the project locally, just to check for sure. Unfortunately, I ran into a bug with graphql-codegen that prevented me from running it. However, you should be able to check yourself by searching all of your client-side javascript in Chrome DevTools. If you search for your secret key (or even just for the string "AWSSECRETKEY", since you used that in the config), then you should get no results.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No result in the search when searching for the key or any of the other server side vars.
So, at least that part seems to be secure.
I still agree that the code quality could be better.

file.uid = getUid(fileName) + "." + fileType;
const isJpgOrPng = file.type === "image/jpeg" || file.type === "image/png";
if (!isJpgOrPng) {
message.error("You can only upload JPG or PNG images!");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this restriction?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just to restrict the file type to an image format. What other image formats should be supported?

As far as I can tell, you can't restrict what type of file is uploaded with a pre signed url.
You can give it a content type, but it will still accept any.
Same with file size. So, thats a problem if the user want to be malicious.
I think a different method would need to be used to enforce file size and type on the S3 end.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see anything wrong with accepting all image types: that is, all MIME types that begin with image/. See the MDN article on MIME types to see some examples.

Copy link
Member

Choose a reason for hiding this comment

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

It should probably be an image format you can render in most browsers unless you're using an avatar resizing helper. You might also want to restrict things like people using GIF avatars because it can be pretty irritating...

@benjie
Copy link
Member

benjie commented May 15, 2020

I tried to get this into a mergeable state in #167 but I ran out of time.

@benjie benjie changed the base branch from master to main June 24, 2020 11:35
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