Javier,
On Tue, Jul 5, 2011 at 6:37 PM, Javier Andalia <janda...@gmail.com> wrote:
> Andres, please read inline,
>
> On Wed, Jun 29, 2011 at 7:37 PM, Andres Riancho
> <andres.rian...@gmail.com> wrote:
>> Javier,
>>
>> I finally got some time to review the changes you've been working
>> on the unicode branch. Here are a couple of comments on them:
>>
>> - branches/unicode/core/data/fuzzer/formFiller.py u'endereço' looks
>> odd for me in Chrome. Is this because of an error in Trac, our file
>> encoding, chrome, a combination of some of those factors?
>
> No idea. It looks good in my chrome. It shouldn't be related to recent
> changes though. Did you try with the version in the trunk?
>
> https://w3af.svn.sourceforge.net/svnroot/w3af/trunk/core/data/fuzzer/formFiller.py
That link looks good to me, the one I was looking the other day
(through Trac) was showing odd stuff. Not important.
>>
>> - branches/unicode/core/data/dc/dataContainer.py DEFAULT_ENCODING =
>> 'utf-8' , shouldn't we have that at a more "global" level?
>>
>
> Actually that's how it is in my local machine. Will update the branch
> with the more recent changes today.
Nice!
>> - " hash_string = hash(httpResponse.body) " , I prefer maintaining the
>> getBody() method
>
> The "getBody" method still exists (mainly because there is a lot of
> other code that uses it). However, is there any particular reason for
> your preference? I see "body" (as many other members of class
> httpResponse) more like a "property" instead of a method. IMHO –taking
> into account that python already supports "properties"– is a more
> elegant approach to use them when make sense.
>
>>
>> - branches/unicode/core/data/dc/queryString.py "queryString =
>> QueryString = dataContainer", not good for debugging and doesn't allow
>> us to do something like "if isinstance(..., queryString)"
>>
>
> You're right. Will rollback that change.
>
>> - branches/unicode/core/data/dc/form.py "def __init__(self,
>> init_val=(), encoding='utf-8'): " maybe that 'utf-8' string should be
>> replaced by DEFAULT_ENCODING?
>>
>
> True!
>
>> - branches/unicode/core/data/request/fuzzableRequest.py , def
>> __str__(self): "result_string.encode(self._dc.encoding, 'replace') "
>> are you sure that this is needed?
>>
>
> What exactly are you asking? This is how that line looks right now:
>
> "return result_string.encode(DEFAULT_ENCODING)"
Not sure what I was thinking at that point, but I think it was related
to the NEED or NO NEED of encoding at that point.
>> - branches/unicode/core/data/request/httpQsRequest.py
>> before: raise ValueError('The URI of a httpQsRequest must be of
>> urlParser.url_object type.')
>> after: raise TypeError('The URI of a httpQsRequest must be of
>> urlParser.url_object type.')
>> Totally agree with the change
>>
>> - branches/unicode/core/data/url/handlers/localCache.py,
>> "postData=str(request.get_data() or '')," why not "unicode(" ?
>
> The function that consumes the data internally expects string objects
Ok,
>> Also, shouldn't all the str() disappear from our code?
>>
>
> Not exactly. The way we should see this unicode change is like this:
> "The goal is to guarantee that everything that enters w3af is decoded
> into unicode, and everything that leaves w3af is encoded to string"
>
> For this case, we are creating requests from "raw"; i.e. bytes strings.
>
>> - class httpResponse(object): @param code: , @param read: needs
>> documentation.
>>
>
> Will add it.
>
>> - We should keep setCharset. The reason behind doing this is that it's
>> easier for us to debug and refactor our code if we can "grep
>> setCharset(.*) * -Rs"
>> 232 @charset.setter
>> 233 def charset(self, charset):
>> 234 self._charset = charset
>> 235
>> 236 def setCharset(self, charset):
>> 237 self.charset = charset
>>
>
> This the same as for the "getBody vs body" change commented above. In
> the other hand, do you really think that keeping the "grep-capability"
> of the method name matters that much?
Yep, I like to be able to find stuff in the code using grep.
> I have deep doubts regarding
> this.
>
>>
>> - Are you sure that this is correct? Don't we use that information for
>> storing to disk or something similar? We might be loosing information
>> here! branches/unicode/core/data/url/httpResponse.py
>> 514 if not self._is_text_or_html_response:
>> 515 body = '<BINARY DATA>'
>>
>
> Let me double check it and go back with this asap.
Ok, we don't want to loose data when exporting/importing a httpResponse.
>> There are many changes, and most of them (at least the ones I saw
>> here [0]) are cosmetic. Which files should I really be looking at?
>> Which files do you want me review?
>>
>
> The same as before. Will throughly look at the changes and list the
> files that really matters
Please do,
>> [0]
>> https://sourceforge.net/apps/trac/w3af/changeset?old_path=%2Fbranches%2Funicode&old=4222&new_path=%2Fbranches%2Funicode&new=4333#file83
>>
>
> Javier
>
--
Andrés Riancho
Director of Web Security at Rapid7 LLC
Founder at Bonsai Information Security
Project Leader at w3af
------------------------------------------------------------------------------
All of the data generated in your IT infrastructure is seriously valuable.
Why? It contains a definitive record of application performance, security
threats, fraudulent activity, and more. Splunk takes this data and makes
sense of it. IT sense. And common sense.
http://p.sf.net/sfu/splunk-d2d-c2
_______________________________________________
W3af-develop mailing list
W3af-develop@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/w3af-develop
