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

API: Make exceptions concrete & include msg #1831

Open
1 task done
ebroda opened this issue Sep 10, 2024 · 1 comment · May be fixed by #1912
Open
1 task done

API: Make exceptions concrete & include msg #1831

ebroda opened this issue Sep 10, 2024 · 1 comment · May be fixed by #1912

Comments

@ebroda
Copy link
Contributor

ebroda commented Sep 10, 2024

Description of the issue

As of #1829 I simply got a PermissionDenied. I wanted to create new table and had never done that before, so I was not sure what was the problem. I checked the code, and there are three possible reasons why the request failed:

oeplatform/api/views.py

Lines 410 to 417 in be12447

if schema not in PLAYGROUNDS and schema not in UNVERSIONED_SCHEMAS:
raise PermissionDenied
if schema.startswith("_"):
raise PermissionDenied
if request.user.is_anonymous:
raise PermissionDenied
if actions.has_table(dict(schema=schema, table=table), {}):
raise APIError("Table already exists")

And there's also a case where a specific exception is thrown.

Ideas of solution

If it's not by intention, to provide less information for attackers, I'd suggest to include more concrete exceptions with a message, explaining the problem.

Concrete example for the one above:

if schema not in PLAYGROUNDS and schema not in UNVERSIONED_SCHEMAS:
    raise APIError('Schema is not in allowed set of schemes for upload')
if schema.startswith("_"):
    raise APIError('Schema starts with _, which is not allowed')
if request.user.is_anonymous:
    raise APIError('User is anonymous', 401)
if actions.has_table(dict(schema=schema, table=table), {}):
    raise APIError("Table already exists", 409)

With that I'm able to check out, okay, I should have a look at this aspect.

If you're basically fine with that change, I could add a PR for that.

Workflow checklist

@jh-RLI
Copy link
Contributor

jh-RLI commented Nov 26, 2024

Sorry for the late reply. I have no problem with it. You can send the PR if you want :)

I think there was some concern about certain things some time ago, but currently I think it's better to make it understandable instead of trying to hide such things from attackers. The data is backed up regularly and should be quite secure, we don't store a lot of user data. I don't think there is much to be gained by attacking the oeplatform.

ebroda added a commit to ebroda/oeplatform that referenced this issue Nov 28, 2024
@ebroda ebroda linked a pull request Nov 28, 2024 that will close this issue
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants