[opensource-dev] Review Request: STORM-1112 Support SOCKS 5 proxy in the viewer (take 2)

Monty Brandenberg monty at lindenlab.com
Tue Jul 12 23:50:51 PDT 2011
Previous message: [opensource-dev] Review Request: STORM-1112 Support SOCKS 5 proxy in the viewer (take 2)
Next message: [opensource-dev] Review Request: STORM-1112 Support SOCKS 5 proxy in the viewer (take 2)
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/374/#review869
-----------------------------------------------------------


This finishes the first-pass review.  Not ready to ship yet.


indra/llmessage/llproxy.h
<http://codereview.secondlife.com/r/374/#comment894>

    Extra semicolon.



indra/llmessage/llproxy.cpp
<http://codereview.secondlife.com/r/374/#comment898>

    Prefer sizeof(password_reply), it avoids
    an unverifiable duplicated name/type
    binding.
    



indra/llmessage/llproxy.cpp
<http://codereview.secondlife.com/r/374/#comment901>

    The security minded would do several things
    at this point:
    
    1.  Scribble over password_auth before deleting it.
    
    2.  Scribble over mSocksPassword now that
    we've used it once.
    
    3.  Narrow the window from retrieving the
    password to this point as much as is
    practical.
    



indra/llmessage/llproxy.cpp
<http://codereview.secondlife.com/r/374/#comment899>

    See above.



indra/llmessage/llproxy.cpp
<http://codereview.secondlife.com/r/374/#comment897>

    Whitespace.



indra/llmessage/llproxy.cpp
<http://codereview.secondlife.com/r/374/#comment900>

    Is anyone doing validation of the strings?
    What happens if they're over 255 bytes?
    Invalid utf-8 encodings?  Do we care?  Are
    these in the test plan?



indra/llmessage/llproxy.cpp
<http://codereview.secondlife.com/r/374/#comment896>

    This block isn't reachable due to 
    preceding 'if' clause.  Also leaves
    'rv' set to success if it failed to 
    write all data for some reason.



indra/llmessage/llproxy.cpp
<http://codereview.secondlife.com/r/374/#comment895>

    Possible to have legal short reads here.



indra/llmessage/net.cpp
<http://codereview.secondlife.com/r/374/#comment902>

    Does net.cpp really need llproxy.h?  I
    don't think it should.



indra/newview/app_settings/settings.xml
<http://codereview.secondlife.com/r/374/#comment914>

    Is this still storing username and
    password in the settings file?
    
    Add this to the test plan.
    



indra/newview/llfloaterpreference.cpp
<http://codereview.secondlife.com/r/374/#comment903>

    Whitespace (indentation & if)
    
    Also, this thing is now 'prefs_proxy',
    yes?
    



indra/newview/llfloaterpreference.cpp
<http://codereview.secondlife.com/r/374/#comment904>

    whitespace



indra/newview/llfloaterpreference.cpp
<http://codereview.secondlife.com/r/374/#comment905>

    Defensive programming on socksAuth being NULL?



indra/newview/llstartup.cpp
<http://codereview.secondlife.com/r/374/#comment906>

    This may be too late.  GPU/features table
    has already been loaded by HTTP, I believe.
    
    Then there's webkit and our browser.
    
    Need to check on this.  (And test plan it.)
    



indra/newview/llstartup.cpp
<http://codereview.secondlife.com/r/374/#comment907>

    There are a bunch of common strings shared
    between this and the UI code ("Web",
    "BrowserProxyEnabled", etc.) that feel
    like they should be #defines somewhere.
    Somewhere not llproxy.h but application/
    ui/interface oriented.
    
    



indra/newview/llstartup.cpp
<http://codereview.secondlife.com/r/374/#comment908>

    Left over from the previous review, I
    think, here's a place where the logic
    can be unsynchronized.  'use_socks_proxy'
    is actually identical to the 'else if'
    test above, isn't it?  These should be
    consistent if I'm reading it correctly.



indra/newview/llstartup.cpp
<http://codereview.secondlife.com/r/374/#comment909>

    All the credential code wants its own
    home, doesn't it?



indra/newview/llstartup.cpp
<http://codereview.secondlife.com/r/374/#comment911>

    What does 'payload' do?



indra/newview/llstartup.cpp
<http://codereview.secondlife.com/r/374/#comment910>

    I'd put this early return after startProxy().
    We're in notification code here, long after
    we know we're okay.
    
    And what happens with an unrecognized status
    code?



indra/newview/llxmlrpctransaction.cpp
<http://codereview.secondlife.com/r/374/#comment912>

    These combined with 328 feel like they
    should be a utility method on mCurlRequest.
    
    Or the whole block a method or
    free function in LLProxy...
    



indra/newview/llxmlrpctransaction.cpp
<http://codereview.secondlife.com/r/374/#comment913>

    Whitespace.



indra/newview/skins/default/xui/en/panel_preferences_setup.xml
<http://codereview.secondlife.com/r/374/#comment915>

    Bad name?
    



indra/newview/skins/default/xui/en/panel_preferences_setup.xml
<http://codereview.secondlife.com/r/374/#comment916>

    Bad 'label_selected' value?


- Monty


On July 12, 2011, 11:20 a.m., Log Linden wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/374/
> -----------------------------------------------------------
> 
> (Updated July 12, 2011, 11:20 a.m.)
> 
> 
> Review request for Viewer, Oz Linden, Monty Brandenberg, and Stone Linden.
> 
> 
> Summary
> -------
> 
> This is a continuation of Robin Cornelius's SOCKS 5 contribution, shown in https://codereview.secondlife.com/r/232/ .  I have tried to address all of the comments on that code review and do as much cleanup as possible. The diff includes everything that was submitted by Robin, as well as my work.
> Major changes since I started working:
> * Changed SOCKS 5 proxy control channel to use the existing LLSocket class, which is a thin wrapper around APR sockets.
> * Worked with the Linden Lab UX team to revamp the proxy controls.
> * Proxy credentials are now stored in the LLSecAPI password storage, which is the same that is used for users' Second Life Credentials instead of as being stored in the clear as a preference.
> 
> 
> This addresses bug STORM-1112.
>     http://jira.secondlife.com/browse/STORM-1112
> 
> 
> Diffs
> -----
> 
>   indra/llmessage/CMakeLists.txt c7a4b7a24e05 
>   indra/llmessage/llcurl.cpp c7a4b7a24e05 
>   indra/llmessage/lliosocket.h c7a4b7a24e05 
>   indra/llmessage/lliosocket.cpp c7a4b7a24e05 
>   indra/llmessage/llpacketring.h c7a4b7a24e05 
>   indra/llmessage/llpacketring.cpp c7a4b7a24e05 
>   indra/llmessage/llproxy.h PRE-CREATION 
>   indra/llmessage/llproxy.cpp PRE-CREATION 
>   indra/llmessage/net.h c7a4b7a24e05 
>   indra/llmessage/net.cpp c7a4b7a24e05 
>   indra/llui/llfunctorregistry.h c7a4b7a24e05 
>   indra/newview/app_settings/settings.xml c7a4b7a24e05 
>   indra/newview/llappviewer.cpp c7a4b7a24e05 
>   indra/newview/llfloaterpreference.h c7a4b7a24e05 
>   indra/newview/llfloaterpreference.cpp c7a4b7a24e05 
>   indra/newview/llloginhandler.cpp c7a4b7a24e05 
>   indra/newview/llpanellogin.h c7a4b7a24e05 
>   indra/newview/llsecapi.h c7a4b7a24e05 
>   indra/newview/llstartup.h c7a4b7a24e05 
>   indra/newview/llstartup.cpp c7a4b7a24e05 
>   indra/newview/llviewerfloaterreg.cpp c7a4b7a24e05 
>   indra/newview/llxmlrpctransaction.cpp c7a4b7a24e05 
>   indra/newview/skins/default/xui/en/floater_preferences_proxy.xml PRE-CREATION 
>   indra/newview/skins/default/xui/en/notifications.xml c7a4b7a24e05 
>   indra/newview/skins/default/xui/en/panel_cof_wearables.xml c7a4b7a24e05 
>   indra/newview/skins/default/xui/en/panel_preferences_privacy.xml c7a4b7a24e05 
>   indra/newview/skins/default/xui/en/panel_preferences_setup.xml c7a4b7a24e05 
> 
> Diff: http://codereview.secondlife.com/r/374/diff
> 
> 
> Testing
> -------
> 
> I've tested exclusively on Linux so far.  I'm working on a more extensive test plan that includes setting up a gateway with a restrictive firewall to verify that all traffic is going through the proxy. 
> 
> Test builds and screenshots of the changed UI elements are available from the project page, located here:
> https://wiki.secondlife.com/wiki/User:Log_Linden/Socks5Viewer
> 
> 
> Thanks,
> 
> Log
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.secondlife.com/pipermail/opensource-dev/attachments/20110713/567592e5/attachment-0001.htm
Previous message: [opensource-dev] Review Request: STORM-1112 Support SOCKS 5 proxy in the viewer (take 2)
Next message: [opensource-dev] Review Request: STORM-1112 Support SOCKS 5 proxy in the viewer (take 2)
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
More information about the opensource-dev mailing list