-
Notifications
You must be signed in to change notification settings - Fork 722
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
WebHost: validate uploaded datapackage and calculate own checksum #2639
Conversation
from . import app | ||
from .models import Seed, Room, Slot, GameDataPackage | ||
|
||
banned_extensions = (".sfc", ".z64", ".n64", ".nes", ".smc", ".sms", ".gb", ".gbc", ".gba") | ||
allowed_options_extensions = (".yaml", ".json", ".yml", ".txt", ".zip") | ||
allowed_generation_extensions = (".archipelago", ".zip") | ||
|
||
games_package_schema = schema.Schema({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- did you look into validating via TypedDict instead of Schema? (we have a TypedDict in
worlds/__init__.py
. I read up on that and the frameworks that support(ed) that were massive, so I gave up on that idea, but I may have missed something) - should we maybe move the Schema to
worlds/__init__.py
so it's local to the TypedDict?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered that we already have a typed dict, but also am not a aware of a small and quick way of validating it. But we do already have schema, so I went with that.
Second, maybe, I considered that too, I ended up moving it to webhost to keep init time low as we avoid a schema import if we don't need it. But I also didn't measure how long the module needs, so this may be over-engineered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should maybe add a code path to actually display the Exception.
I get
Internal Server Error
The server encountered an unexpected internal server error
(generated by waitress)
But validation itself seems to work, so approving for now.
there's multiple things that lead to a generic "internal server error" especially on that page. I agree that we should improve that at some point. |
What is this fixing or adding?
validate uploaded datapackage and calculate own checksum
How was this tested?
Turned the actual ID of "Donald Lucky Lucky" into None. Then got:
If this makes graphical changes, please attach screenshots.