Generalized the find_build service's parameters

[Aaron1011]

No description provided.

[joshk]

we should probably whitelist params which are accepted, as this could become a DDOS vector by creating slow queries, and also potentially a security attack vector.

[sarahhodne]

I think just params.slice would do, no need to raise an error if more params are passed (most, if not all, other services just ignore additional params).

[Aaron1011]

@henrikhodne @joshk: Does this look good?

[sarahhodne]

I'd use params.slice(ALLOWED_PARAMS) instead.

[sarahhodne]

That might have to be params.slice(*ALLOWED_PARAMS), now that I think about it.

[joshk]

👍 to slice

On 13/09/2013, at 12:03 AM, Henrik Hodne notifications@github.com wrote:

In lib/travis/services/find_build.rb:

@@ -30,7 +32,7 @@ def all_resources
end

     def result

•      @result ||= scope(:build).find_by_id(params[:id])
  

•      @result ||= scope(:build).where(params.select { |k| ALLOWED_PARAMS.include? k.to_sym } ).first
  

  That might have to be params.slice(*ALLOWED_PARAMS), now that I think about it.

—
Reply to this email directly or view it on GitHub.

[Aaron1011]

@henrikhodne @joshk: Okay, fixed

[joshk]

Are all those params currently used for the where search? Does it make sense to allow all these? Especially as the previous implementation was find_by_id?

[Aaron1011]

@joshk: I need to add at least owner_id and pull_request_number for my other PR to cancel builds when a PR is closed. I don't think the others are necessary right now, so we could remove them.

[joshk]

Can you point me to the related PRs again.

[Aaron1011]

@joshk: Here it is. The parameters to cancel_build get passed along to find_build.

[joshk]

I can't see owner_id being used here, and although pull_request_number is needed, it feels like the interaction can be encapsulated a little better. I need to think about this a little. Will look at it more this week.

[Aaron1011]

@joshk: Ping

[Aaron1011]

@joshk: Ping