Bug 1408555

This was fixed in Bastion by https://github.com/Katello/bastion/pull/139 - will link the issues.

Connecting redmine issue http://projects.theforeman.org/issues/17291 from this bug

Setting as triaged as there's a fix already upstream

Please add verifications steps for this bug to help QE verify

Steps to verify:

1) register host(s) to a capsule
2) go to capsules/show page for that capsule
3) in the "Overview" tab, notice there is a correct number of "Hosts managed"
4) click the link, it should lead to a search on a hosts/index page showing the list of hosts registered to the capsule

Created attachment 1268725 [details]
Error when clicking overview filter

When I clicked on "Hosts Managed" on "Overview" page I got an error "Error: Field 'smart_proxy+' not recognized for searching!". I attached "filter_error.jpg" screen shot with it.

The page had the filter 'smart_proxy+=+"some_fqdn"'. When I changed it to 'smart_proxy="some_fqdn"' it worked as expected. So it seems a matter of removing plus signs from the filter (+)

Upstream bug assigned to oprazak

Walden, this sound very familiar to some other issue you fixed IIRC, does this ring any bells?

(In reply to Tomer Brisker from comment #15)
> Walden, this sound very familiar to some other issue you fixed IIRC, does
> this ring any bells?

This is on foreman host pages right?  The katello JS that caused a similar (and since fixed) bug wouldn't even be loaded on these pages.  I vaguely remember the bug you are referring to but I couldn't find it readily.

From Renzo's comment#13 it looks like these +s are being correctly encoded to %2B in the link.

(In reply to Walden Raines from comment #16)
> (In reply to Tomer Brisker from comment #15)
> > Walden, this sound very familiar to some other issue you fixed IIRC, does
> > this ring any bells?
> 
> This is on foreman host pages right?  The katello JS that caused a similar
> (and since fixed) bug wouldn't even be loaded on these pages.  I vaguely
> remember the bug you are referring to but I couldn't find it readily.
> 
> From Renzo's comment#13 it looks like these +s are being correctly encoded
> to %2B in the link.

Found it!

https://github.com/Katello/bastion/pull/105

This is on the smart proxy page, so bastion is loaded there. 
It looks like the "+" signs are escaped in this case when they shouldn't be - they are part of the url used to concat the %3d of the equal sign. Might be that Ondrej's patch to bypass the angular routing also caused it to bypass your fix?

(In reply to Tomer Brisker from comment #18)
> This is on the smart proxy page, so bastion is loaded there. 
> It looks like the "+" signs are escaped in this case when they shouldn't be
> - they are part of the url used to concat the %3d of the equal sign. Might
> be that Ondrej's patch to bypass the angular routing also caused it to
> bypass your fix?

To concat the %3d?  What do you mean?  Why not just decode the url server side? Both %2B and + should behave the same in my opinion (although I don't really even understand why the + is needed in the first place).

My fix was more of a hack to workaround this issue with foreman.  Ideally all special characters should be encoded (I think the RFC even specifies this) but the server should be able to accept both.

(In reply to Tomer Brisker from comment #18)
> Might be that Ondrej's patch to bypass the angular routing also caused it to
> bypass your fix?

To answer your question, yes, that is what is happening.  Ondrej's change bypasses my hack.

I mean that we encode the url twice - once gets us the +%3d+ part (encoding the "=" sign) and the second time tries to encode the "+"s which causes the url to break.

Tomer/Walden.. next steps here?

Oh I see.  But +%3d+ isn't correct encoding for = by itself unless there are spaces (i.e. " = ").  What is the original URL, are there spaces?  The correct url encoding for "=" is "%3d".

As far as next steps, if there are no spaces in the URL then we should fix our encoding if there are spaces then I guess we'll have to add this hack to the capsule URL as well.

The correct URL may contain spaces - even if we strip them from around the "=" sign they are still possible inside the proxy name. 
It looks like Ondrej's patch seems to make angular only ignore links inside the page itself (i.e. those that match 'smart_proxies') so there may be other broken links as well in there (I noticed the link in puppet environments also exhibits the same behaviour). 
So looks like we need your patch applied on that page as well.

bkearney - I'm not sure if this constitutes a blocker for 6.2.9, I'd be happy to get it fixed but I don't think I'd hold the release for it.

My point is that "+%3d+" decodes to " = " and I'm not sure that's what we want.  In fact, I'm not even sure that is a valid query string.  

If you go to your browser and type in 'http://google.com/search?q = blah' look what happens as opposed to 'http://google.com/search?q=blah' (notice that the +s also don't work).  I believe that +s are only valid in query string keys or names (i.e. 'http://someUrl.com?key+name=value+with+space' would result in 'http://someUrl.com?key name=value with space').

Using +s are only valid in application/x-www-form-urlencoded strings such as query string keys and values.  We should not be using spaces or +s around the equals sign.  

How difficult would it be to remove the spaces/+s from around the =s signs?

it's a valid query, note that the "=" is part of the search, not another request parameter - the url is /host?query=smart_proxy+%3d+proxyname so that the = as well as any other special signs (such as spaces) need to be url-encoded correctly. As I said in comment #24, the proxy name can also include spaces, so even if we get rid of these two we will just be moving the problem to somewhere else, so we need to handle spaces correctly and not double-encode them. TBH I'm not sure why angular tries to re-encode urls at all, but I guess that's beyond the scope of this issue.

(In reply to Tomer Brisker from comment #26)
> it's a valid query, note that the "=" is part of the search, not another
> request parameter - the url is /host?query=smart_proxy+%3d+proxyname so that
> the = as well as any other special signs (such as spaces) need to be
> url-encoded correctly. As I said in comment #24, the proxy name can also
> include spaces, so even if we get rid of these two we will just be moving
> the problem to somewhere else, so we need to handle spaces correctly and not
> double-encode them. TBH I'm not sure why angular tries to re-encode urls at
> all, but I guess that's beyond the scope of this issue.

Oh I see, sorry about the confusion, I thought smart_proxy was the query key.

Then yes, this seems like a bug with how angular is (re)encoding the URL.  I would be happy to take a look at this for 6.3.  I agree with Tomer that this shouldn't be a blocker for 6.2.9

Per comment 27, I am moving htis out of 6.2.9.

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHBA-2017:1191

Looks like this was mistakenly marked as closed in 6.2.9, it has not been fixed yet. Reopening.

*** Bug 1459119 has been marked as a duplicate of this bug. ***

*** Bug 1459299 has been marked as a duplicate of this bug. ***

I do not know what changes were made, but this seems to be fixed in 6.3. Could QE verify?

*** Bug 1329541 has been marked as a duplicate of this bug. ***

*** Bug 1481953 has been marked as a duplicate of this bug. ***

ON_QA status is for 6.3, but there exists a bug for 6.3. Moving this to NEW for 6.2.

This will be fixed in 6.3, and I do not see this being backported into 6.2.z. If you have concerns with this, please feel free to reach out to me.