Never place an OpenID provider into an iframe It prevents the user from validating the security and identity of their OpenID provider, making it easier for a phishing site to get the user's identity information. Bug: GERRIT-102 Signed-off-by: Shawn O. Pearce <sop@google.com>
diff --git a/src/main/java/com/google/gerrit/UserAgent.gwt.xml b/src/main/java/com/google/gerrit/UserAgent.gwt.xml index b032f29..05d3d4b 100644 --- a/src/main/java/com/google/gerrit/UserAgent.gwt.xml +++ b/src/main/java/com/google/gerrit/UserAgent.gwt.xml
@@ -1,11 +1,4 @@ <module> - <replace-with class="com.google.gerrit.client.openid.AllowFrameImplSafari"> - <when-type-is class="com.google.gerrit.client.openid.AllowFrameImpl" /> - <any> - <when-property-is name="user.agent" value="safari"/> - </any> - </replace-with> - <replace-with class="com.google.gerrit.client.ui.FancyFlexTableImplIE6"> <when-type-is class="com.google.gerrit.client.ui.FancyFlexTableImpl" /> <any>
diff --git a/src/main/java/com/google/gerrit/client/openid/AllowFrameImpl.java b/src/main/java/com/google/gerrit/client/openid/AllowFrameImpl.java deleted file mode 100644 index 6f58c8b..0000000 --- a/src/main/java/com/google/gerrit/client/openid/AllowFrameImpl.java +++ /dev/null
@@ -1,32 +0,0 @@ -// Copyright (C) 2009 The Android Open Source Project -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.gerrit.client.openid; - -class AllowFrameImpl { - boolean permit(final String url) { - if (OpenIdUtil.URL_GOOGLE.equals(url) - || url.startsWith(OpenIdUtil.URL_GOOGLE + "?")) { - return true; - } - if (is_claimID(url)) { - return true; - } - return false; - } - - protected static boolean is_claimID(final String url) { - return url.contains(".claimid.com/") || url.contains("://claimid.com/"); - } -}
diff --git a/src/main/java/com/google/gerrit/client/openid/AllowFrameImplSafari.java b/src/main/java/com/google/gerrit/client/openid/AllowFrameImplSafari.java deleted file mode 100644 index e0db8b0..0000000 --- a/src/main/java/com/google/gerrit/client/openid/AllowFrameImplSafari.java +++ /dev/null
@@ -1,26 +0,0 @@ -// Copyright (C) 2009 The Android Open Source Project -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.gerrit.client.openid; - -class AllowFrameImplSafari extends AllowFrameImpl { - @Override - boolean permit(final String url) { - if (is_claimID(url)) { - return false; - } - - return super.permit(url); - } -}
diff --git a/src/main/java/com/google/gerrit/client/openid/OpenIdLoginPanel.java b/src/main/java/com/google/gerrit/client/openid/OpenIdLoginPanel.java index 0ab4bb6..480a64a 100644 --- a/src/main/java/com/google/gerrit/client/openid/OpenIdLoginPanel.java +++ b/src/main/java/com/google/gerrit/client/openid/OpenIdLoginPanel.java
@@ -25,7 +25,6 @@ import com.google.gwt.user.client.DeferredCommand; import com.google.gwt.user.client.Event; import com.google.gwt.user.client.History; -import com.google.gwt.user.client.Timer; import com.google.gwt.user.client.Window; import com.google.gwt.user.client.ui.AbstractImagePrototype; import com.google.gwt.user.client.ui.Button; @@ -52,9 +51,7 @@ public class OpenIdLoginPanel extends Composite implements FormHandler { private final SignInDialog.Mode mode; private final CallbackHandle<?> discoveryCallback; - private final CallbackHandle<?> signInCallback; private final LoginIcons icons; - private final AllowFrameImpl allowFrame; private final FlowPanel panelWidget; private final FormPanel form; private final FlowPanel formBody; @@ -76,7 +73,6 @@ public OpenIdLoginPanel(final SignInDialog.Mode m, final CallbackHandle<?> sc) { mode = m; - signInCallback = sc; discoveryCallback = OpenIdUtil.SVC.discover(new GerritCallback<DiscoveryResult>() { @@ -85,7 +81,6 @@ } }); icons = GWT.create(LoginIcons.class); - allowFrame = GWT.create(AllowFrameImpl.class); formBody = new FlowPanel(); formBody.setStyleName("gerrit-OpenID-loginform"); @@ -97,7 +92,7 @@ providerFrame = new NamedFrame(DOM.createUniqueId()); providerFrame.setVisible(false); - form = new FormPanel(providerFrame); + form = new FormPanel(); form.setMethod(FormPanel.METHOD_GET); form.setAction(GWT.getModuleBaseURL() + "login"); form.addFormHandler(this); @@ -284,31 +279,12 @@ redirectBody.add(new Hidden(e.getKey(), e.getValue())); } - final FormElement fe = FormElement.as(redirectForm.getElement()); - if (allowFrame.permit(url)) { - // The provider will work OK inside an IFRAME, so use it. - // - fe.setTarget(providerFrame.getName()); - - // The provider page needs time to load. It won't load as fast as - // we can update the DOM so we delay our DOM update for just long - // enough that the provider is likely to be loaded. - // - new Timer() { - @Override - public void run() { - panelWidget.remove(form); - providerFrame.setVisible(true); - } - }.schedule(250); - } else { - // The provider won't support operation inside an IFRAME, so we - // replace our entire application. No fancy waits are needed, - // the browser won't update anything until its started to load - // the provider's page. - // - fe.setTarget("_top"); - } + // The provider won't support operation inside an IFRAME, so we + // replace our entire application. No fancy waits are needed, + // the browser won't update anything until its started to load + // the provider's page. + // + FormElement.as(redirectForm.getElement()).setTarget("_top"); redirectForm.submit(); } else { @@ -348,28 +324,7 @@ discoveryCallback.install(); discoveryCbField.setValue("parent." + discoveryCallback.getFunctionName()); providerField.setValue(url); - - if (allowFrame.permit(url)) { - signInCbField.setValue("parent." + signInCallback.getFunctionName()); - } else { - // The provider won't work right inside of an IFRAME (or likely isn't - // going to work within the IFRAME) so we need to replace the whole - // application and then redirect back to this location. - // - signInCbField.setValue("history:" + History.getToken()); - } - } - - @Override - public void setWidth(final String width) { - providerFrame.setWidth(width); - super.setWidth(width); - } - - @Override - public void setHeight(final String height) { - providerFrame.setHeight(height); - super.setHeight(height); + signInCbField.setValue("history:" + History.getToken()); } public void onSubmitComplete(final FormSubmitCompleteEvent event) {