[gwt-contrib] Fix: http://code.google.com/p/google-web-toolkit/issues/detail?id=6367 (issue1508802)
Reviewers: tobyr, jat, Description: Fix: http://code.google.com/p/google-web-toolkit/issues/detail?id=6367 Create shared utils package and include it in gwt-dev and gwt-servlet Please review this at http://gwt-code-reviews.appspot.com/1508802/ Affected files: M dev/core/src/com/google/gwt/dev/ExternalPermutationWorkerFactory.java M dev/core/src/com/google/gwt/dev/util/Util.java M dev/core/src/com/google/gwt/util/tools/Utility.java A dev/core/src/com/google/gwt/util/tools/shared/Md5Utils.java A dev/core/src/com/google/gwt/util/tools/shared/StringUtils.java A dev/core/src/com/google/gwt/util/tools/shared/package-info.java M servlet/build.xml M user/src/com/google/gwt/i18n/rebind/keygen/MD5KeyGenerator.java M user/src/com/google/gwt/i18n/server/keygen/MD5KeyGenerator.java M user/src/com/google/gwt/user/server/rpc/XsrfProtectedServiceServlet.java M user/src/com/google/gwt/user/server/rpc/XsrfTokenServiceServlet.java -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: This change adds couple of things: (issue1251801)
http://gwt-code-reviews.appspot.com/1251801/diff/44001/45010 File user/src/com/google/gwt/user/server/Util.java (right): http://gwt-code-reviews.appspot.com/1251801/diff/44001/45010#newcode76 user/src/com/google/gwt/user/server/Util.java:76: * @throws IllegalStateException if duplicate cookies are detected. On 2011/01/13 18:53:45, jat wrote: I think either IllegalStateException or IllegalArgumentException is fine -- the state of the request is in error, and that request was passed as an argument. I agree it isn't worth creating a custom exception for it. I've changed to IllegalArgumentException http://gwt-code-reviews.appspot.com/1251801/diff/44001/45014 File user/src/com/google/gwt/user/server/rpc/XsrfProtectedServiceServlet.java (right): http://gwt-code-reviews.appspot.com/1251801/diff/44001/45014#newcode37 user/src/com/google/gwt/user/server/rpc/XsrfProtectedServiceServlet.java:37: * XSRF token validation is performed by generating MD5 hash of the session On 2011/01/13 18:31:42, xtof wrote: Actually, why MD5 and not future proof things and use SHA-1 or SHA-2? I'm not c ryptographer enough to know if it really matters (and my guess is collision resi stance is not important in this application), but better err on the safe side? I don't feel strongly about the algorithm used here, since I think we don't really care about collisions here. It seems like the attack would be for two session cookies to hash to the same xsrf token, in which case attacker would have to find second person logged in with the same cookie. Overwriting session cookie to result in collision will hopefully be detected by the web app and hindered by the browsers restrictions on allowed characters, etc. http://gwt-code-reviews.appspot.com/1251801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: This change adds couple of things: (issue1251801)
http://gwt-code-reviews.appspot.com/1251801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: This change adds couple of things: (issue1251801)
http://gwt-code-reviews.appspot.com/1251801/diff/30001/31010
File user/src/com/google/gwt/user/server/Util.java (right):
http://gwt-code-reviews.appspot.com/1251801/diff/30001/31010#newcode76
user/src/com/google/gwt/user/server/Util.java:76: if (cookieName == null
|| request == null) {
On 2011/01/12 19:22:55, xtof wrote:
On 2011/01/12 19:17:43, jat wrote:
Precondition isn't part of GWT, and this seems too insignificant to
add
another
external dependency.
Oh right, I forgot (I've seen it in GWT internal code iirc).
Anyway, the main point is that this method perhaps should fail with a
NPE if its
arguments are null, rather than silently returning null. Though I
don't feel
strongly about that either.
Removed if statements to let it fail with a NPE.
http://gwt-code-reviews.appspot.com/1251801/diff/30001/31010#newcode84
user/src/com/google/gwt/user/server/Util.java:84: // ensure that it's
the only one cookie, since duplicate cookies
On 2011/01/12 19:04:15, xtof wrote:
Since this check (and the exception thrown if it fails) is specific to
RpcTokens, it might be good to change the name of the method to
something less
generic?
Change the exception to IllegalStateException and added boolean
parameter to control this behaviour.
http://gwt-code-reviews.appspot.com/1251801/diff/30001/31018
File user/test/com/google/gwt/user/client/rpc/XsrfProtectionTest.java
(right):
http://gwt-code-reviews.appspot.com/1251801/diff/30001/31018#newcode60
user/test/com/google/gwt/user/client/rpc/XsrfProtectionTest.java:60:
public void testRpcWithoutXsrfToken() throws Exception {
On 2011/01/12 19:04:15, xtof wrote:
This should perhaps be called testRpcWithoutXsrfTokenFails?
Done.
http://gwt-code-reviews.appspot.com/1251801/diff/30001/31018#newcode60
user/test/com/google/gwt/user/client/rpc/XsrfProtectionTest.java:60:
public void testRpcWithoutXsrfToken() throws Exception {
On 2011/01/12 19:04:15, xtof wrote:
Also, perhaps add one more test with an RPC token that's present but
wrong?
This is pretty much covered by the test below with a changed session
cookie, but
it is a slightly different scenario and probably deserves its own
test.
Done.
http://gwt-code-reviews.appspot.com/1251801/diff/30001/31018#newcode127
user/test/com/google/gwt/user/client/rpc/XsrfProtectionTest.java:127:
public void testXsrfTokenWithSessionCookie() throws Exception {
On 2011/01/12 19:04:15, xtof wrote:
should perhaps be called testXsrfTokenWithDifferentSessionCookieFails?
Done.
http://gwt-code-reviews.appspot.com/1251801/diff/30001/31021
File
user/test/com/google/gwt/user/server/rpc/AbstractXsrfProtectedServiceServletTest.java
(right):
http://gwt-code-reviews.appspot.com/1251801/diff/30001/31021#newcode31
user/test/com/google/gwt/user/server/rpc/AbstractXsrfProtectedServiceServletTest.java:31:
private boolean isValidateCalled = false;
On 2011/01/12 19:04:15, xtof wrote:
perhaps set this to false in setUp()?
Done.
http://gwt-code-reviews.appspot.com/1251801/diff/30001/31023
File user/test/com/google/gwt/user/server/rpc/XsrfTestServiceImpl.java
(right):
http://gwt-code-reviews.appspot.com/1251801/diff/30001/31023#newcode32
user/test/com/google/gwt/user/server/rpc/XsrfTestServiceImpl.java:32:
StringBuffer person = new StringBuffer();
On 2011/01/12 19:04:15, xtof wrote:
One thing you could do perhaps: change this method so that it
actually has a
side effect (i.e., make 'person' a field), and then check in the tests
where
XSRF token validation failed, this method did not actually get invoked
(i.e.,
the state change did not happen).
Done.
http://gwt-code-reviews.appspot.com/1251801/diff/30001/31023#newcode37
user/test/com/google/gwt/user/server/rpc/XsrfTestServiceImpl.java:37: //
public void setSessionCookieName(String cookieName) {
On 2011/01/12 19:04:15, xtof wrote:
Perhaps just delete this?
Done.
http://gwt-code-reviews.appspot.com/1251801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: This change adds couple of things: (issue1251801)
http://gwt-code-reviews.appspot.com/1251801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: This change adds couple of things: (issue1251801)
http://gwt-code-reviews.appspot.com/1251801/diff/11001/12009 File user/src/com/google/gwt/user/server/rpc/XsrfTokenServiceServlet.java (right): http://gwt-code-reviews.appspot.com/1251801/diff/11001/12009#newcode192 user/src/com/google/gwt/user/server/rpc/XsrfTokenServiceServlet.java:192: setCookieAndExpireOldCookies(newXsrfCookie); On 2011/01/11 06:10:23, xtof wrote: On 2011/01/11 01:48:15, meder wrote: On 2011/01/11 00:37:06, xtof wrote: If I understand this correctly, the cookie is set both when the xsrf token is a random value, _and_ when the token is generated off of a SessionCookie. In the latter case, I don't understand why it's necessary to set the XSRF cookie, and if it's not necessary I think it should be avoided. I thought that this would make it easier for apps to work with token, app would have to only issue getNewXsrfToken() once and subsequently simply read the value from the cookie and construct XsrfToken that way. I see. Well typically I'd think a GWT client would just call the getNewXsrfToken rpc and hang on to the token in client state; I'm not sure if it needs to be in a cookie. Come to think of it, if we do provide infrastructure code that stores values in cookies, we should make it configurable with respect to path of the cookie and 'secure' attribute. Which in turn seems to introduce a fair bit of complication. Plus, if a developer really wants to store the value in a cookie, they can do so. I've removed cookie setting as well as the mode where XSRF token wasn't tied to session cookie. Please take another look. http://gwt-code-reviews.appspot.com/1251801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: This change adds couple of things: (issue1251801)
http://gwt-code-reviews.appspot.com/1251801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: This change adds couple of things: (issue1251801)
http://gwt-code-reviews.appspot.com/1251801/diff/11001/12005
File
user/src/com/google/gwt/user/server/rpc/AbstractXsrfProtectedServiceServlet.java
(right):
http://gwt-code-reviews.appspot.com/1251801/diff/11001/12005#newcode80
user/src/com/google/gwt/user/server/rpc/AbstractXsrfProtectedServiceServlet.java:80:
!method.equals(classMethods)) {
On 2011/01/11 00:32:23, xtof wrote:
Shouldn't this be
!method.equals(classMethod))
(not classMethod*s*)?
Seems like there's a test case missing for this?
Awesome catach! There was a test
AbstractXsrfProtectedServiceServletTest:120, but it had a bug too.
Fixed.
http://gwt-code-reviews.appspot.com/1251801/diff/11001/12008
File
user/src/com/google/gwt/user/server/rpc/XsrfProtectedServiceServlet.java
(right):
http://gwt-code-reviews.appspot.com/1251801/diff/11001/12008#newcode39
user/src/com/google/gwt/user/server/rpc/XsrfProtectedServiceServlet.java:39:
* To prevent blind XSRF cookie overwrite by an active HTTP
man-in-the-middle in
On 2011/01/11 00:32:23, xtof wrote:
I think it would be worth stating more clearly that using the default
XSRF
cookie is vulnerable to active attacks, and is not recommended. I'm
almost
inclined to suggest that we should perhaps not even provide that
default mode.
After all, pretty much any app that worries about XSRF also uses
authentication,
and the XSRF token can be derived off of a session cookie or similar.
I am open to the idea of removing default XSRF protection and only
supporting session cookie derived tokens.
John, what do you think?
http://gwt-code-reviews.appspot.com/1251801/diff/11001/12008#newcode120
user/src/com/google/gwt/user/server/rpc/XsrfProtectedServiceServlet.java:120:
private boolean isCookieValueValid(String cookieValue) {
On 2011/01/11 00:32:23, xtof wrote:
I'm not too fond of how this method performs two different kinds of
functions,
depending on how the class is configured:
If the class is configured to use a session cookie, this method seems
to check
that the token value is as expected, whereas if no sessionCookieName
is set, it
just checks for well-formedness of the cookie.
Either way, I'm a bit confused as to how this works...
This method only checks if the value itself is sane code on line 107
will compare the two values.
http://gwt-code-reviews.appspot.com/1251801/diff/11001/12009
File
user/src/com/google/gwt/user/server/rpc/XsrfTokenServiceServlet.java
(right):
http://gwt-code-reviews.appspot.com/1251801/diff/11001/12009#newcode192
user/src/com/google/gwt/user/server/rpc/XsrfTokenServiceServlet.java:192:
setCookieAndExpireOldCookies(newXsrfCookie);
On 2011/01/11 00:37:06, xtof wrote:
If I understand this correctly, the cookie is set both when the xsrf
token is a
random value, _and_ when the token is generated off of a
SessionCookie. In the
latter case, I don't understand why it's necessary to set the XSRF
cookie, and
if it's not necessary I think it should be avoided.
I thought that this would make it easier for apps to work with token,
app would have to only issue getNewXsrfToken() once and subsequently
simply read the value from the cookie and construct XsrfToken that way.
http://gwt-code-reviews.appspot.com/1251801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: This change adds couple of things: (issue1251801)
http://gwt-code-reviews.appspot.com/1251801/diff/11001/12010
File user/src/com/google/gwt/user/server/rpc/XsrfUtils.java (right):
http://gwt-code-reviews.appspot.com/1251801/diff/11001/12010#newcode62
user/src/com/google/gwt/user/server/rpc/XsrfUtils.java:62: public static
T extends Annotation T getClassAnnotation(Class? clazz,
On 2011/01/06 18:52:37, jat wrote:
Rather than copying, I would prefer to simply move it to a more
central location
if you don't want to just use it where it is.
As discussed over IM, AnnotationUtil use JClassType and requires oracle.
http://gwt-code-reviews.appspot.com/1251801/diff/11001/12010#newcode85
user/src/com/google/gwt/user/server/rpc/XsrfUtils.java:85: * consistency
in duplicate cookies handling.
On 2011/01/06 18:52:37, jat wrote:
I don't understand this comment -- why does being package-private help
consistency? Also, it seems like it isn't package-private -- is this
just an
outdated comment?
Yep, outdated comment. Fixed.
http://gwt-code-reviews.appspot.com/1251801/diff/11001/12010#newcode124
user/src/com/google/gwt/user/server/rpc/XsrfUtils.java:124: public
static String getMd5DigestHexString(byte[] input) {
On 2011/01/06 18:52:37, jat wrote:
Use Util.computeStrongName instead of recreating it here.
This brings in a bunch of dependencies (TreeLogger, Utility, TypeOracle,
UnableToCompleteException, etc) to servlet-impl, is that ok?.
Alternatively, I think it'll make sense to have MD5Utils, similar to
Base64Utils in com.google.gwt.user.server, what do you think?
http://gwt-code-reviews.appspot.com/1251801/diff/11001/12015
File user/test/com/google/gwt/user/client/rpc/XsrfTestServiceAsync.java
(right):
http://gwt-code-reviews.appspot.com/1251801/diff/11001/12015#newcode23
user/test/com/google/gwt/user/client/rpc/XsrfTestServiceAsync.java:23:
void setSessionCookieName(String cookieName, AsyncCallbackVoid
callback);
On 2011/01/06 18:52:37, jat wrote:
Should we detect if annotations are placed on the Async interface
instead of the
sync one? That seems like an error that could be easily made, and it
would
result in possibly no protection where it was expected.
I think if we decide to perform those checks then they should not only
be performed for Async interfaces but also on RPC servlets.
Alternatively, we could have an app context initialization parameter
gwt.xsrf.enable_on_all_RPCs, which would enforce XSRF protection on
all RPCs and would help catch misplaced annotations.
Thoughts?
http://gwt-code-reviews.appspot.com/1251801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: This change adds couple of things: (issue1251801)
On 2011/01/05 00:01:40, jat wrote:
LGTM except for nits. However, I haven't thoroughly reviewed the
tests yet, so
I want to look over them further to make sure everything is covered
that needs
to be.
Sounds good.
Also, we need to get everyone to agree about adding this new API
before this can
be committed.
Ok.
http://gwt-code-reviews.appspot.com/1251801/diff/1/5
File
user/src/com/google/gwt/user/server/rpc/AbstractXsrfProtectedServiceServlet.java
(right):
http://gwt-code-reviews.appspot.com/1251801/diff/1/5#newcode2
user/src/com/google/gwt/user/server/rpc/AbstractXsrfProtectedServiceServlet.java:2:
* Copyright 2010 Google Inc.
2011, and elsewhere in new files
http://gwt-code-reviews.appspot.com/1251801/diff/1/5#newcode35
user/src/com/google/gwt/user/server/rpc/AbstractXsrfProtectedServiceServlet.java:35:
* of that interface except for the method returning {...@link
RpcToken}.
I presume this is only if there are no annotations at all, right?
Should
clarify.
http://gwt-code-reviews.appspot.com/1251801/diff/1/9
File
user/src/com/google/gwt/user/server/rpc/XsrfTokenServiceServlet.java
(right):
http://gwt-code-reviews.appspot.com/1251801/diff/1/9#newcode68
user/src/com/google/gwt/user/server/rpc/XsrfTokenServiceServlet.java:68:
*
public interface MyRpcService extends RemoteService {
Should there be an XsrfProtectedService that just inherits from
RemoteService
and has the annotation to make this parallel to the server side?
http://gwt-code-reviews.appspot.com/1251801/diff/1/9#newcode228
user/src/com/google/gwt/user/server/rpc/XsrfTokenServiceServlet.java:228:
return
XsrfUtils.getMd5DigestHexString(bytes);
Suggest moving return to after the if/else, and setting bytes below
instead of
the return.
http://gwt-code-reviews.appspot.com/1251801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: This change adds couple of things: (issue1251801)
http://gwt-code-reviews.appspot.com/1251801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: This change adds couple of things: (issue1251801)
http://gwt-code-reviews.appspot.com/1251801/diff/1/5
File
user/src/com/google/gwt/user/server/rpc/AbstractXsrfProtectedServiceServlet.java
(right):
http://gwt-code-reviews.appspot.com/1251801/diff/1/5#newcode2
user/src/com/google/gwt/user/server/rpc/AbstractXsrfProtectedServiceServlet.java:2:
* Copyright 2010 Google Inc.
On 2011/01/05 00:01:41, jat wrote:
2011, and elsewhere in new files
Done.
http://gwt-code-reviews.appspot.com/1251801/diff/1/5#newcode35
user/src/com/google/gwt/user/server/rpc/AbstractXsrfProtectedServiceServlet.java:35:
* of that interface except for the method returning {...@link
RpcToken}.
On 2011/01/05 00:01:41, jat wrote:
I presume this is only if there are no annotations at all, right?
Should
clarify.
Done.
http://gwt-code-reviews.appspot.com/1251801/diff/1/9
File
user/src/com/google/gwt/user/server/rpc/XsrfTokenServiceServlet.java
(right):
http://gwt-code-reviews.appspot.com/1251801/diff/1/9#newcode68
user/src/com/google/gwt/user/server/rpc/XsrfTokenServiceServlet.java:68:
* public interface MyRpcService extends RemoteService {
On 2011/01/05 00:01:41, jat wrote:
Should there be an XsrfProtectedService that just inherits from
RemoteService
and has the annotation to make this parallel to the server side?
Good idea. Done.
http://gwt-code-reviews.appspot.com/1251801/diff/1/9#newcode228
user/src/com/google/gwt/user/server/rpc/XsrfTokenServiceServlet.java:228:
return XsrfUtils.getMd5DigestHexString(bytes);
On 2011/01/05 00:01:41, jat wrote:
Suggest moving return to after the if/else, and setting bytes below
instead of
the return.
Done.
http://gwt-code-reviews.appspot.com/1251801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: This change adds couple of things: (issue1251801)
http://gwt-code-reviews.appspot.com/1251801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add support for RpcTokens, which, if set, are sent with each RPCRequest to (issue1107801)
http://gwt-code-reviews.appspot.com/1107801/diff/3001/4005 File user/src/com/google/gwt/user/client/rpc/ServiceDefTarget.java (right): http://gwt-code-reviews.appspot.com/1107801/diff/3001/4005#newcode43 user/src/com/google/gwt/user/client/rpc/ServiceDefTarget.java:43: RpcToken getRpcToken(); On 2010/11/23 00:17:28, jat wrote: Ok, how about extracting the new methods into an interface HasRpcToken. It shouldn't be much of a burden on calling code anyway. Done. http://gwt-code-reviews.appspot.com/1107801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add support for RpcTokens, which, if set, are sent with each RPCRequest to (issue1107801)
http://gwt-code-reviews.appspot.com/1107801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add support for RpcTokens, which, if set, are sent with each RPCRequest to (issue1107801)
http://gwt-code-reviews.appspot.com/1107801/diff/3001/4005 File user/src/com/google/gwt/user/client/rpc/ServiceDefTarget.java (right): http://gwt-code-reviews.appspot.com/1107801/diff/3001/4005#newcode43 user/src/com/google/gwt/user/client/rpc/ServiceDefTarget.java:43: RpcToken getRpcToken(); On 2010/11/22 19:33:42, jat wrote: On 2010/11/22 03:52:05, meder wrote: On 2010/11/20 04:33:44, xtof wrote: How likely is it that developers will be providing their own implementations of ServiceDefTarget? We'd be breaking them by adding methods here. I've seen it being done, so this will most definitely break some code. I'm waiting for someone from GWT team to look at this and tell me if this is fine or if I should add another interface. Do you have any reference to non-generated code that implements ServiceDefTarget? I would expect only mocks would do so. It's mostly mocks but not all: http://codesearch.google.com/codesearch?hl=enlr=q=%22implements+ServiceDefTarget%22+lang:javasbtn=Search That said, it wouldn't be much of a problem to put the RpcToken APIs on another interface. Either way is fine with me. http://gwt-code-reviews.appspot.com/1107801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add support for RpcTokens, which, if set, are sent with each RPCRequest to (issue1107801)
http://gwt-code-reviews.appspot.com/1107801/diff/3001/4005
File user/src/com/google/gwt/user/client/rpc/ServiceDefTarget.java
(right):
http://gwt-code-reviews.appspot.com/1107801/diff/3001/4005#newcode43
user/src/com/google/gwt/user/client/rpc/ServiceDefTarget.java:43:
RpcToken getRpcToken();
On 2010/11/20 04:33:44, xtof wrote:
How likely is it that developers will be providing their own
implementations of
ServiceDefTarget? We'd be breaking them by adding methods here.
I've seen it being done, so this will most definitely break some code.
I'm waiting for someone from GWT team to look at this and tell me if
this is fine or if I should add another interface.
http://gwt-code-reviews.appspot.com/1107801/diff/3001/4009
File user/src/com/google/gwt/user/rebind/rpc/ProxyCreator.java (right):
http://gwt-code-reviews.appspot.com/1107801/diff/3001/4009#newcode109
user/src/com/google/gwt/user/rebind/rpc/ProxyCreator.java:109:
RpcToken.Class tokenClassToUse =
remoteService.findAnnotationInTypeHierarchy(
On 2010/11/20 04:33:44, xtof wrote:
80 columns
Hmm...this is older version of code, but anyhow, this was fixed.
http://gwt-code-reviews.appspot.com/1107801/diff/13001/14002
File user/src/com/google/gwt/user/client/rpc/RpcToken.java (right):
http://gwt-code-reviews.appspot.com/1107801/diff/13001/14002#newcode28
user/src/com/google/gwt/user/client/rpc/RpcToken.java:28: public
interface RpcToken extends Serializable {
On 2010/11/20 04:33:44, xtof wrote:
As mentioned in the draft design, I'm not sure RpcToken is the best
name for
this. It's really just a value that's implicitly passed along with
each RPC.
Maybe, RpcRequestHeader? But really I'd defer to GWT team for advice
on naming
here...
If everyone is ok with the proposed name, I can change it.
http://gwt-code-reviews.appspot.com/1107801/diff/13001/14006
File
user/src/com/google/gwt/user/client/rpc/impl/AbstractSerializationStream.java
(right):
http://gwt-code-reviews.appspot.com/1107801/diff/13001/14006#newcode82
user/src/com/google/gwt/user/client/rpc/impl/AbstractSerializationStream.java:82:
if (((flags | VALID_FLAGS_MASK) ^ VALID_FLAGS_MASK) == 0) {
On 2010/11/20 04:33:44, xtof wrote:
Why not just return (that expression)?
True! Done.
http://gwt-code-reviews.appspot.com/1107801/diff/13001/14008
File
user/src/com/google/gwt/user/client/rpc/impl/RemoteServiceProxy.java
(right):
http://gwt-code-reviews.appspot.com/1107801/diff/13001/14008#newcode276
user/src/com/google/gwt/user/client/rpc/impl/RemoteServiceProxy.java:276:
protected void checkRpcTokenType(RpcToken token) {
On 2010/11/20 04:33:44, xtof wrote:
Perhaps javadoc?
Done.
http://gwt-code-reviews.appspot.com/1107801/diff/13001/14010
File user/src/com/google/gwt/user/rebind/rpc/ProxyCreator.java (right):
http://gwt-code-reviews.appspot.com/1107801/diff/13001/14010#newcode192
user/src/com/google/gwt/user/rebind/rpc/ProxyCreator.java:192:
stob.addRootType(logger, icseType);
On 2010/11/20 04:33:44, xtof wrote:
Spurious whitespace at line-end.
Done.
http://gwt-code-reviews.appspot.com/1107801/diff/13001/14020
File user/test/com/google/gwt/user/client/rpc/RpcTokenTest.java (right):
http://gwt-code-reviews.appspot.com/1107801/diff/13001/14020#newcode81
user/test/com/google/gwt/user/client/rpc/RpcTokenTest.java:81:
token.tokenValue = Drink kumys!;
On 2010/11/20 04:33:44, xtof wrote:
Yikes!
Yum!
http://gwt-code-reviews.appspot.com/1107801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add support for RpcTokens, which, if set, are sent with each RPCRequest to (issue1107801)
http://gwt-code-reviews.appspot.com/1107801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add support for RpcTokens, which, if set, are sent with each RPCRequest to (issue1107801)
On Thu, Nov 18, 2010 at 6:23 AM, Ray Ryan rj...@google.com wrote: Meder, are you able to take on the RequestFactory task? Yes I can do it, should it be in a separate CL? -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add support for RpcTokens, which, if set, are sent with each RPCRequest to (issue1107801)
On Thu, Nov 18, 2010 at 6:35 AM, BobV b...@google.com wrote: I guess the big question is whether or not the XSRF protection is a function of the payload envelope or needs deeper support. If it can be done at the transport layer, extending DefaultRequestTransport seems like easy way to mix it in. RPC XSRF protection allows for arbitrary serializable implementations to be sent, I'll take a look at the DefaultRequestTransport and see where would be the best place to add this. I'll get on it once this code is in. Meder -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add support for RpcTokens, which, if set, are sent with each RPCRequest to (issue1107801)
I haven't look at RequestFactory, but it should be possible to have RequestFactory propagate the object to RequestContext and include it in the payload. On Tue, Nov 16, 2010 at 2:01 PM, BobV b...@google.com wrote: Has anyone looked at applying this to RequestFactory? Would it be sufficient for the RequestTransport to maintain the token state? -- Bob Vawter Google Web Toolkit Team -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add support for RpcTokens, which, if set, are sent with each RPCRequest to (issue1107801)
http://gwt-code-reviews.appspot.com/1107801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add support for RpcTokens, which, if set, are sent with each RPCRequest to (issue1107801)
http://gwt-code-reviews.appspot.com/1107801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add support for RpcTokens, which, if set, are sent with each RPCRequest to (issue1107801)
http://gwt-code-reviews.appspot.com/1107801/diff/1/7
File
user/src/com/google/gwt/user/client/rpc/impl/AbstractSerializationStream.java
(right):
http://gwt-code-reviews.appspot.com/1107801/diff/1/7#newcode52
user/src/com/google/gwt/user/client/rpc/impl/AbstractSerializationStream.java:52:
public static final int SERIALIZATION_STREAM_MIN_VERSION = 5;
On 2010/11/15 16:02:21, jat wrote:
I think the version number needs to be bumped, since I don't believe
the
implementation fails if any unknown flag bits are set.
Version bumped.
To avoid having to bump the version in similar cases in the future, it
should
probably check if any unknown flags values are set and fail the same
was as a
protocol version mismatch.
Added checks for unknown flags.
However, I remember complications when Dan Rice increased the version
number for
changing the long representation, so we should look back over those
and see if
we can avoid the problems.
Any pointers for this?
http://gwt-code-reviews.appspot.com/1107801/diff/3001/4002
File user/src/com/google/gwt/user/client/rpc/RpcToken.java (right):
http://gwt-code-reviews.appspot.com/1107801/diff/3001/4002#newcode37
user/src/com/google/gwt/user/client/rpc/RpcToken.java:37: public
@interface Class {
On 2010/11/15 16:02:21, jat wrote:
Since usually people will have this imported, I would prefer a better
name -
@Class on an interface isn't going to be clear what it refers to. How
about
@RpcTokenImplementation?
Done.
http://gwt-code-reviews.appspot.com/1107801/diff/3001/4008
File
user/src/com/google/gwt/user/client/rpc/impl/RequestCallbackAdapter.java
(right):
http://gwt-code-reviews.appspot.com/1107801/diff/3001/4008#newcode170
user/src/com/google/gwt/user/client/rpc/impl/RequestCallbackAdapter.java:170:
String methodName, RpcStatsContext statsContext, AsyncCallbackT
callback,
On 2010/11/15 16:02:21, jat wrote:
Line length 80.
Done.
http://gwt-code-reviews.appspot.com/1107801/diff/3001/4008#newcode172
user/src/com/google/gwt/user/client/rpc/impl/RequestCallbackAdapter.java:172:
this(streamFactory, methodName, statsContext, callback, null,
responseReader);
On 2010/11/15 16:02:21, jat wrote:
Line length 80.
Done.
http://gwt-code-reviews.appspot.com/1107801/diff/3001/4008#newcode230
user/src/com/google/gwt/user/client/rpc/impl/RequestCallbackAdapter.java:230:
} else if (tokenExceptionHandler != null caught instanceof
RpcTokenException) {
On 2010/11/15 16:02:21, jat wrote:
Line length.
Done.
http://gwt-code-reviews.appspot.com/1107801/diff/3001/4009
File user/src/com/google/gwt/user/rebind/rpc/ProxyCreator.java (right):
http://gwt-code-reviews.appspot.com/1107801/diff/3001/4009#newcode189
user/src/com/google/gwt/user/rebind/rpc/ProxyCreator.java:189:
stob.addRootType(logger, rteType);
On 2010/11/15 16:02:21, jat wrote:
Should this be conditional on finding an RpcToken subtype?
Done.
http://gwt-code-reviews.appspot.com/1107801/diff/3001/4009#newcode497
user/src/com/google/gwt/user/rebind/rpc/ProxyCreator.java:497: //
setRpcToken()
On 2010/11/15 16:02:21, jat wrote:
Should this check instead be done at setRpcToken time? Perhaps have
setRpcToken
call protected boolean checkRpcTokenType(RpcToken) which by default
returns
true, and generate a check there?
I know I previously said over chat that a cast was fine, but thinking
about it
more I think we need a better error here. So, I think you want an
instanceof
check and then throw an exception with more details about the problem.
Done.
http://gwt-code-reviews.appspot.com/1107801/diff/3001/4009#newcode593
user/src/com/google/gwt/user/rebind/rpc/ProxyCreator.java:593:
returnStatement.append(return new +
FailingRequestBuilder.class.getName() + (
On 2010/11/15 16:02:21, jat wrote:
If you are going to use a StringBuffer here, don't mix regular + --
use append
to add the pieces together.
Done.
http://gwt-code-reviews.appspot.com/1107801/diff/3001/4009#newcode605
user/src/com/google/gwt/user/rebind/rpc/ProxyCreator.java:605:
w.println(} catch(ClassCastException + exceptionName + ) {);
On 2010/11/15 16:02:21, jat wrote:
I think if the RpcToken type is checked at setRpcToken time, all of
these
changes aren't needed, right?
Done.
http://gwt-code-reviews.appspot.com/1107801/diff/3001/4010
File user/src/com/google/gwt/user/server/rpc/HybridServiceServlet.java
(right):
http://gwt-code-reviews.appspot.com/1107801/diff/3001/4010#newcode137
user/src/com/google/gwt/user/server/rpc/HybridServiceServlet.java:137:
log(An RpcTokenException was thrown while processing this call.,
tokenException);
On 2010/11/15 16:02:21, jat wrote:
Line length.
Done.
http://gwt-code-reviews.appspot.com/1107801/diff/3001/4018
File user/test/com/google/gwt/user/client/rpc/RpcTokenTest.java (right):
http://gwt-code-reviews.appspot.com/1107801/diff/3001/4018#newcode67
user/test/com/google/gwt/user/client/rpc/RpcTokenTest.java:67: fail();
On 2010/11/15 16:02:21, jat wrote:
Should include the
