Skip to content
This repository has been archived by the owner on Feb 8, 2018. It is now read-only.

Commit

Permalink
Validate elsewhere username in failure page
Browse files Browse the repository at this point in the history
  • Loading branch information
chadwhitacre committed Dec 3, 2014
1 parent a9e4c80 commit 4d7d0dc
Showing 1 changed file with 1 addition and 0 deletions.
1 change: 1 addition & 0 deletions www/on/%platform/%user_name/failure.html.spt
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ if platform is None:
user_name = path['user_name']
if not user_name:
raise Response(400, '%user_name is empty')
platform.get_user_info(user_name) # raises 404 if user_name is unknown

This comment has been minimized.

Copy link
@techtonik

techtonik Dec 3, 2014

Contributor

I don't like implicit 404 error rise here.

platform here is elsewhere.Platform, and elsewhere is something that I'd expect to be independent of Aspen. At least I can clearly see a situation where this check could be made from command line script.

This comment has been minimized.

Copy link
@techtonik

techtonik Dec 3, 2014

Contributor

It is used only in account_elsewhere.py and in associate.spt.

And speaking about account_elsewhere.py there is already get_account_elsewhere() helper that contains much of the same code.

This comment has been minimized.

Copy link
@chadwhitacre

chadwhitacre Dec 3, 2014

Author Contributor

I don't like implicit 404 error rise here.

Do you think we should block this PR on a refactor of elsewhere?

This comment has been minimized.

Copy link
@techtonik

techtonik Dec 3, 2014

Contributor

Looks like these calls will result in external lookups. Is that it? I thought we only need to check that the name is present in current DB, no? In which cases we need to show failure page for non-registered Twitter user?

This comment has been minimized.

Copy link
@chadwhitacre

chadwhitacre Dec 3, 2014

Author Contributor

Yes, external lookup as currently implemented. We could check locally in our db as well, I suppose. This is a low-priority ticket and I think this implementation is fine for now. I don't think we'll soak our ratelimit because of a lookup here, since this page won't be hit very often.

This comment has been minimized.

Copy link
@techtonik

techtonik via email Dec 3, 2014

Contributor

This comment has been minimized.

Copy link
@techtonik

techtonik via email Dec 3, 2014

Contributor

This comment has been minimized.

Copy link
@chadwhitacre

chadwhitacre Dec 3, 2014

Author Contributor

Okay, cool.

This comment has been minimized.

Copy link
@chadwhitacre

chadwhitacre Dec 3, 2014

Author Contributor

It can be called by unauthenticated users, and it is potential attack vector to deplete resources.

This is true of the /on/%platform/%user_name/index.html.spt page. We call get_account_elsewhere, which will always make an external API call if user_name is unknown on platform.

This comment has been minimized.

Copy link
@chadwhitacre

chadwhitacre Dec 3, 2014

Author Contributor

I'd limit checks only to local db.

Done in f8d00a8.


try:
action = qs['action']
Expand Down

8 comments on commit 4d7d0dc

@techtonik
Copy link
Contributor

Choose a reason for hiding this comment

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

Where path['platform'] comes from?

@chadwhitacre
Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the %platform part of the URL path. See http://aspen.io/virtual-paths/.

@techtonik
Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. Is there a list of all other automagical variables that Aspen/Simplates set that I need to know about?

@techtonik
Copy link
Contributor

Choose a reason for hiding this comment

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

(in particular I wonder why Response is not automagical as well)

@chadwhitacre
Copy link
Contributor Author

Choose a reason for hiding this comment

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

(in particular I wonder why Response is not automagical as well)

You mean the Response class and not the local response object, correct? Not a bad idea. Ticketed as AspenWeb/pando.py#402.

@chadwhitacre
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a list of all other automagical variables that Aspen/Simplates set that I need to know about?

The presence of the path magic variable is documented at http://aspen.io/api/request/. But see AspenWeb/pando.py#227.

@chadwhitacre
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a list of all other automagical variables that Aspen/Simplates set that I need to know about?

We don't have a list of all Simplate builtins in one place, and we should.

@techtonik
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have a list of all Simplate builtins in one place, and we should.

Added AspenWeb/pando.py#403

Please sign in to comment.