358588 - The sslbase's port is harcoded, but shouldn't (allow the port to be specified with the parameter)

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Martijn Ras]

Heya Folks,

When one specifies a port in the URL for 'sslbase' under "Required Settings" the system doesn't accept that value, it gives the following message:

Invalid Parameter
The new value for sslbase is invalid: must not contain a port..

Steps to reproduce
1) Log in to a Bugzilla account with administrator rights
2) Click on 'Parameters' on the bottom of the page
3) Fill the 'sslbase' field with "https://123.456.789.0:1234"
4) Click on 'Save Changes' on the bottom of the page

The parameter 'urlbase' does accept the URL so everything seems to be working find so far. Still i think the parameter 'sslbase' should accept a port in the URL as well.

Mazzel,

Martijn.

Comment 1

[timeless]

this was clearly intentional.

Comment 2

[timeless]

Attachment: maybe something like this would work

just some random code.

Comment 3

[Dave Miller [:justdave]]

(In reply to comment #1)
> this was clearly intentional.

There's absolutely no discussion on bug 260682 about why it was intentional, and there's no obvious reason for it to be.  I'd say it was a misunderstanding about how SSL works, or someone was assuming it only worked on port 443.  I think it'd be perfectly reasonable to allow a port there (we allow ports in urlbase don't we?)

Comment 4

[Dave Miller [:justdave]]

Comment on attachment 243970 [details] [diff] [review]
maybe something like this would work

not sure why we need both the Socket and LWP tests... one or the other should be sufficient.  LWP is likely to be more accurate since that'll actually verify that the connection actually uses SSL and not that there's something listening on it.  But LWP is an awfully big set of perl modules to require once you count all its dependencies...  do we already require it for something else?  I don't think it'd be worth requiring just for this if not.

Comment 5

[:glob ✱]

oops :)

what about checking with LWP only if the LWP::UserAgent and Crypt::SSLeay modules are installed, otherwise just use the socket test?

Comment 6

[timeless]

Attachment: maybe something like this is what is wanted?

note that this patch is just a munged version of the previous patch. it might not apply to anything.

Comment 7

[:glob ✱]

Comment on attachment 244168 [details] [diff] [review]
maybe something like this is what is wanted?

>+ if (eval('use Crypt::SSLeay; use LWP::UserAgent;') {

eval doesn't return success/failure, check $@


otherwise the logic is good with me.

Comment 8

[timeless]

Attachment: ok, so does this work?

Comment 9

[Dave Miller [:justdave]]

Comment on attachment 244183 [details] [diff] [review]
ok, so does this work?

no, your error check is backwards.  This says if we failed to load LWP, then try to use it anyway, and if we succeed in loading it, then don't use it. :)

Comment 10

[timeless]

Attachment: ok, so how about this?

Comment 11

[Frédéric Buclin]

Comment on attachment 244861 [details] [diff] [review]
ok, so how about this?

>+        unless ($@) {

>+        } else {

Is unless... else... really a valid combination?

Comment 12

[Frédéric Buclin]

What happens if you are behind a proxy? Bugzilla/Update.pm takes care of that, and probably you should do this check here as well.

Comment 13

[Yann Forget]

Hello,

This bug is really annoying. It obliges to dirty hacks with Apache Rewriting rules if a non standard port is needed.

Yann

Comment 14

[Frédéric Buclin]

Attachment: patch, v n+1

No need to use LWP. Simply call getservbyname() to get the correct port for HTTPS.

Comment 15

[Frédéric Buclin]

Comment on attachment 244861 [details] [diff] [review]
ok, so how about this?

>+        if ($host =~ /:(\d+)$/) {
>+            $port = $1;
>         }

>+            my $sin = sockaddr_in($port, inet_aton($host));

For the record, this patch won't work as you have to drop the port from the hostname, else inet_aton() throws an error.

Comment 16

[:glob ✱]

Comment on attachment 311792 [details] [diff] [review]
patch, v n+1

looks good to me, r=glob

Comment 17

[Frédéric Buclin]

Checking in Bugzilla/Config/Common.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/Common.pm,v  <--  Common.pm
new revision: 1.21; previous revision: 1.20
done

Comment 18

[Max Kanat-Alexander]

Added to the release notes for Bugzilla 3.2 in a patch on bug 432331.