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.

Sign up for GitHub

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

Jump to bottom

"Use Tls" setting is no longer present in Settings #43

Closed
oxaronick opened this issue Jan 22, 2024 · 4 comments
Closed

"Use Tls" setting is no longer present in Settings #43

oxaronick opened this issue Jan 22, 2024 · 4 comments

Comments

@oxaronick
Copy link
Contributor

Describe the bug

In version 2.6.9 and several versions prior, There was a "Use Tls" setting that was added in response to #28. That setting is no longer present.

To Reproduce

  1. upgrade Twinny to 2.6.13
  2. try to use it with a server that is listening for https instead of plain http

Expected behavior

Twinny should send the request over https.

Actual behaviour

Twinny sends the request over plain http.

Here is a capture of the request sent by Twinny:

POST /api/generate HTTP/1.1
Content-Type: application/json
Authorization: Bearer xxxxxx-xxxxxxxx-xxxxxx-xxx-xxx
Host: my.domain.com:444
Connection: close
Transfer-Encoding: chunked

3f3d
{"model":"codellama:13b-code","prompt":"<PRE> \n\n// Language: Javascript (javascript) \n// File uri: file:///Users/someguy/.vscode/extensions/rjmacarthy.twinny-2.6.13/out/extension.js (javascript) \n          getFileHeader(e, t) {\n            const n = a.languages[e];\n            return n\n              ? `\\n${n.comment?.start || \"\"} Language: ${n.name} (${e}) ${\n                  n.comment?.end || \"\"\n                }\\n${n.comment?.start || \"\"} File uri: ${t.toString()} (${e}) ${\n                  n.comment?.end || \"\"\n                }\\n`\n              : \"\";\n          }\n          calculateSimilarity(e, t) {\n            const n = e.split(\"/\"),\n              i = t.split(\"/\"),\n              o = n[n.length - 1],\n              s = i[i.length - 1];\n            return (\n              n.slice(0, -1).join(\"/\").score(i.slice(0, -1).join(\"/\"), 0.5) +\n              o.score(s, 0.5)\n            );\n          }\n          getFileContext(e) {\n            const t = [],\n              n = e.toString();\n            for (const e of i.workspace.textDocuments) {\n              if (\n

and here is the TLS-terminating proxy's response:

HTTP/1.1 400 Bad Request
Server: nginx
Date: Mon, 22 Jan 2024 17:03:06 GMT
Content-Type: text/html
Content-Length: 248
Connection: close
Strict-Transport-Security: max-age=63072000

<html>
<head><title>400 The plain HTTP request was sent to HTTPS port</title></head>
<body>
<center><h1>400 Bad Request</h1></center>
<center>The plain HTTP request was sent to HTTPS port</center>
<hr><center>nginx</center>
</body>
</html>

Looking at the extension code, I could see the extension examining a URL to see if it started with "http" or "https", but setting the URL to "https://my.domain.com" resulted in "invalid URL" errors from the extension:

2024-01-22 11:56:58.476 [error] ProxyResolver#addCertificatesV1 TypeError: Invalid URL
	at new NodeError (node:internal/errors:399:5)
	at Url.parse (node:url:425:17)
	at Object.urlParse (node:url:147:13)
	at useProxySettings (/Applications/Visual Studio Code.app/Contents/Resources/app/node_modules.asar/@vscode/proxy-agent/out/index.js:121:35)
	at /Applications/Visual Studio Code.app/Contents/Resources/app/node_modules.asar/@vscode/proxy-agent/out/index.js:113:13
	at /Applications/Visual Studio Code.app/Contents/Resources/app/node_modules.asar/@vscode/proxy-agent/out/index.js:459:13

Editing the extension code to always require('https') makes Twinny send the request over https.
@rjmacarthy
Copy link
Owner

rjmacarthy commented Jan 22, 2024 •
edited

Hi @rcgtnick I updated the code to use the baseUrl now, so if the url matches https then https will be used.

Edit: I see you mentioned it, I will look into it.

@rjmacarthy
Copy link
Owner

Hey @rcgtnick I re-introduced the ollamaUseTls option. Relying on https in the hostname seems like not the best way. I hope that fixes it for you?

@oxaronick
Copy link
Contributor Author

Thanks, that's working again!

@oxaronick
Copy link
Contributor Author

Tested in 2.6.17, btw.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@rjmacarthy @oxaronick