Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Secure cookies #1167

Merged
merged 2 commits into from Dec 14, 2016
Merged

Secure cookies #1167

merged 2 commits into from Dec 14, 2016

Conversation

flaix
Copy link
Member

@flaix flaix commented Dec 14, 2016

Gitblit already set the "secure" flag for cookies, i.e. send them only over secured transports, as well as 'httpOnly' flag for the Jetty session cookies already.
With this PR it will also do so for the user authentication cookies.

It will also set the 'secure' flag when HTTP is redirected to HTTPS by Gitblit.

Mark the user authentication cookie to be only used for HTTP, making
it inaccessible for JavaScript engines.


If only HTTPS is used and no HTTP (i.e. also if HTTP is redirected to
HTTPS) then mark the user cookie to be sent only over secure connections.
So far for session cookies the secure property was only set when no
HTTP port was opened. This changes to also set it when HTTP is redirected
to the HTTPS port.
@flaix flaix added this to the 1.9.0 milestone Dec 14, 2016
@gitblit
Copy link
Collaborator

gitblit commented Dec 14, 2016

👍

@gitblit gitblit merged commit 4ece397 into gitblit-org:master Dec 14, 2016
@tomposmiko
Copy link

It fails to compile:

compile:
  [mx:keys] =========================================================
  [mx:keys] mx:Keys  (com.gitblit.Keys)
  [mx:keys] ---------------------------------------------------------
  [mx:keys]    input: /data/src/gitblit/src/main/distrib/data/defaults.properties
  [mx:keys]    output: /data/src/gitblit/src/main/java/com/gitblit/Keys.java
 [mx:javac] =========================================================
 [mx:javac] mx:Javac  (com.gitblit:gitblit:1.8.1-SNAPSHOT, compile)
 [mx:javac] ---------------------------------------------------------
 [mx:javac] Cleaning apt source directory src/main/gen
 [mx:javac] Cleaning output directory /data/src/gitblit/build/classes
 [mx:javac] Compiling 457 source files to /data/src/gitblit/build/classes
 [mx:javac] /data/src/gitblit/src/main/java/com/gitblit/models/UserModel.java:40: error: a type with the same simple name is already defined by the single-type-import of SecureRandom
 [mx:javac] import com.gitblit.utils.SecureRandom;
 [mx:javac] ^
 [mx:javac] /data/src/gitblit/src/main/java/com/gitblit/models/UserModel.java:669: error: cannot find symbol
 [mx:javac] 		return StringUtils.getSHA1(RANDOM.randomBytes(32));
 [mx:javac] 		                                 ^
 [mx:javac]   symbol:   method randomBytes(int)
 [mx:javac]   location: variable RANDOM of type SecureRandom
 [mx:javac] Note: Some input files use or override a deprecated API.
 [mx:javac] Note: Recompile with -Xlint:deprecation for details.
 [mx:javac] Note: Some input files use unchecked or unsafe operations.
 [mx:javac] Note: Recompile with -Xlint:unchecked for details.
 [mx:javac] 2 errors

BUILD FAILED
/data/src/gitblit/build.xml:110: Compile failed; see the compiler error output for details.

@gitblit
Copy link
Collaborator

gitblit commented Dec 15, 2016

Hey Tamas, the compile problem is not related to this PR. It does look like we'll need to review the class name and import collision on the SecureRandom class recently introduced.

@tomposmiko
Copy link

Do you want me to submit a separate issue?

@gitblit
Copy link
Collaborator

gitblit commented Dec 15, 2016

@tomposmiko Good idea.

@flaix
Copy link
Member Author

flaix commented Dec 15, 2016

@tomposmiko, don't bother, it is a quick fix. Already pushed to master.

@tomposmiko
Copy link

Thanks, I didn't have time until now:)

@flaix flaix deleted the secureCookies branch June 16, 2019 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants