-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Update user agent and log URL #11852
Conversation
@@ -384,7 +384,7 @@ public URLConnection openConnection() throws FetcherException { | |||
} else if (status >= 400) { | |||
// in case of an error, propagate the error message | |||
SimpleHttpResponse httpResponse = new SimpleHttpResponse(httpURLConnection); | |||
LOGGER.info("{}", httpResponse); | |||
LOGGER.info("{}: {}", this.source, httpResponse); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will leak api keys? I remember that we had an issue about this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick fix at 390d2ce
WTF, one needs to enable TLS 1.2 manually?! -- implemented at 0f3b387 |
According to https://de.wikipedia.org/wiki/Transport_Layer_Security it should matter if TLS 1.2 or TLS 1.3. I saw a green test "fullTextFoundByDOI" at ResearchGateTest. But only once 😥. |
Note sure if we really need it - https://stackoverflow.com/a/73097062/873282 - but that I saw the green test... Not sure how to proceed. I would keep this change and see if there are any changes (to the better or to the worse). The other alternative is to check which protocols are used etc. |
try { | ||
HttpsURLConnection.setDefaultSSLSocketFactory(socketFactory); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will have an impact of using custom SSL certifactes now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked. --> TrustStore is created at org.jabref.logic.net.ssl.TrustStoreManager#configureTrustStore.
I created JavaDoc for it: 300b7b3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, maybe, this only works if we do this setting?! - I am not sure. How can we test this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I see it should be enough if we set in the truststore manager
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was about to revert the change ^^. -- Since its an alpha, we can take the risk?
can you take a look at the bvb fetcher? java.lang.IllegalArgumentException: Expected authority at index 8: https:// |
.withField(StandardField.AUTHOR, "Bloch, Joshua") | ||
.withField(StandardField.TITLEADDON, "Joshua Bloch") | ||
.withField(StandardField.EDITION, "3. Auflage, Übersetzung der englischsprachigen 3. Originalausgabe 2018") | ||
.withField(StandardField.FILE, "ParsedFileField{description='', link='http://search.ebscohost.com/login.aspx?direct=true&scope=site&db=nlebk&db=nlabk&AN=1906353', fileType='PDF'}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better use withFiles(List.of(new LInkedFile(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was a bug in the fetcher. Fixed at 654616c.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more: d1c33d9
The build for this PR is no longer available. Please visit https://builds.jabref.org/main/ for the latest build. |
This updates the user agent to the latest one by FireFox (source: https://www.whatismybrowser.com/guides/the-latest-user-agent/firefox)
This also logs the URL used for an error HTTP response.
Example:
Mandatory checks
CHANGELOG.md
described in a way that is understandable for the average user (if applicable)