Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

do not allow invalid addresses to succeed in parse_ip #523

Merged
merged 7 commits into from Apr 11, 2013

Conversation

Projects
None yet
4 participants
Contributor

andrewjstone commented Apr 4, 2013

I don't think this is ready to merge. There may be other corner cases here. Needs more testing as well. It's here just to allow a quick look at my approach.

Contributor

andrewjstone commented Apr 4, 2013

This pull is towards a fix for #476

@kuenishi kuenishi and 2 others commented on an outdated diff Apr 8, 2013

src/riak_cs_s3_policy.erl
@@ -744,8 +748,12 @@ condition_({<<"aws:SecureTransport">>, MaybeBool}) ->
{ok, Bool} = parse_bool(MaybeBool),
{'aws:SecureTransport', Bool};
condition_({<<"aws:SourceIp">>, Bin}) when is_binary(Bin)->
- IP = parse_ip(Bin),
- {'aws:SourceIp', IP};
+ case parse_ip(Bin) of
+ {error, _} ->
+ {error, malformed_policy_condition};
@kuenishi

kuenishi Apr 8, 2013

Contributor

In case if you don't know yet, just to be sure: When you create a new error message, you must add response generating code to riak_cs_s3_response.erl.

@andrewjstone

andrewjstone Apr 8, 2013

Contributor

Thanks @kuenishi I was wondering if something like that existed. I'll add it in.

@kuenishi

kuenishi Apr 8, 2013

Contributor

Also, to make this fail, errors should be thrown. I designed like that due to the deep call stack (something like maybe monad would be much better).

        {error, _} -> 
            throw({error, malformed_policy_condition});

After adding this code, in my local env it worked.

@andrewjstone

andrewjstone Apr 8, 2013

Contributor

I'm not so sure I agree with that. I had a conversation with @reiddraper last week about this type of thing and we decided that it was best to keep functions pure when possible unless there was a real measurable performance hit. The root caller (policy_from_json/1) actually does check for an error being returned from this, and the parse_bool function also returns an error from the same condition_/1 function.

@reiddraper

reiddraper Apr 8, 2013

Contributor

Yeah assuming it's not prohibitive to return error tuples, I'd vote for that. I haven't looked at this code, but if it's a deep call stack that never previously accounted for error tuples being returned, exceptions may be the simplest hack for now. Whatever you guys think is best here.

Contributor

andrewjstone commented Apr 8, 2013

Besides the minor disagreement on handling errors, I think this PR is good to go.

@ghost ghost assigned andrewjstone Apr 8, 2013

I thin you need to add an accompanying status_code clause too, otherwise it defaults to 503.

Contributor

andrewjstone replied Apr 8, 2013

whoops. Just added. Thanks.

Contributor

reiddraper commented Apr 8, 2013

@kuenishi you reviewing this?

Contributor

kuenishi commented Apr 9, 2013

@reiddraper yeah, I was just commenting as an original writer (who wrote the bad code:) but now I'd love to.

Contributor

kuenishi commented Apr 9, 2013

@andrewjstone Would be much better if a test hitting http interface added - but currently it's not easy to add policy interface to erlcloud, so I've been adding python boto tests for regression. See BucketPolicyTest in client_tests/python/boto_test.py.

@kuenishi kuenishi was assigned Apr 9, 2013

Contributor

andrewjstone commented Apr 9, 2013

@kuenishi I agree, it needs a test. I will add it in the morning. Thanks for the pointer.

-Andrew

On Mon, Apr 8, 2013 at 8:10 PM, UENISHI Kota notifications@github.comwrote:

@andrewjstone https://github.com/andrewjstone Would be much better if a
test hitting http interface added - but currently it's not easy to add
policy interface to erlcloud, so I've been adding python boto tests for
regression. See BucketPolicyTest in client_tests/python/boto_test.py.


Reply to this email directly or view it on GitHubhttps://github.com/basho/riak_cs/pull/523#issuecomment-16086984
.

Contributor

kuenishi commented Apr 10, 2013

+1 to merge, thanks!

  • riak_test passed (thanks, Kaz!)
  • eunit including eqc passed
  • python client test passed

@andrewjstone andrewjstone merged commit 48701fc into develop Apr 11, 2013

@andrewjstone andrewjstone deleted the bugfix/ipv4strict branch Apr 11, 2013

Contributor

andrewjstone commented Apr 11, 2013

Thanks for the review @kuenishi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment