Should be @kind problem; see https://github.com/github/codeql/blob/master/docs/query-metadata-style-guide.md.

We generally discourage the use of toString() in queries (cf https://github.com/github/codeql-go/blob/main/ql/docs/experimental.md). In this case, I think you want

This alert message doesn't match our usual style. I would suggest you try and rephrase it in a more neutral tone, perhaps something like

We don't normally check in commented-out code.

Alert queries should select two things: a program element, and an alert message.

Could you rename this variable, please?

oh sorry i forgot to change it

It's fine for experimental queries not to have a .qhelp file, so if you don't quite know yet how to best explain what your query does, you can simply remove the .qhelp.

It seems like you're not actually using this variable for anything.

which one？ do you mean x? infor?or tmp?
here is an explanation:
x : i found there is so many attributes in cookie, but you can't locate the "httponly" attributes by an extractly index，so i use a loop in [1 .. 20] to get the attributes call "httponly"
infor : this is easy, the information that i want to give to users
tmp: this is a liite complex to explain, i use "concat" to combine all of the attributes into a string, so i could check if the developer set "httponly" attributes or not

The variable f, which seems to be the only variable mentioned on this line. You are not using it for anything, so you might as well drop it.

x : i found there is so many attributes in cookie, but you can't locate the "httponly" attributes by an extractly index，so i use a loop in [1 .. 20] to get the attributes call "httponly"

There are no loops in CodeQL. When you say s.getKey(x) below, then that is enough to assign a value to x; you don't need to constrain it further, so you can just drop the x in [1..20] bit. (As mentioned below, you'll then also want to restrict the scope of x to the first part of the or by using an exists.)

I don't understand what this query does.

this rule is about the developer use a wrong way to check the signature that come from the user or web end
try to image a sceen like this,
url like this /check.php?para1=abc&para2=def&sign=xxxxxx
para1 and para2 is parameter that come from and can controled by users, and sign is the signature of them like sha1(para1+para2) or something others like this
but if the backend(server) ensure the signature easily just by calculate them like sha1(para1+) and compare it with the signature from web, this would be dangerous.

this would be show easily and dangerous in some payment scene，here is an example
you are buying some from ebay, such as 10 books, and each one is 10 dollers, the total count of them is 100 dollers
usually, web end needs send count and each books money to back end, and also a sign of them
just like /buy.php?count=10&perMoney=10&sign=xxxxx here sign = sha1(count+perMoney)
if the server calculate it by sha1(count+perMoney) as the web do, it could be bypass easily
because count + perMoney = 1010, if the hacker interrupt the request（burp or something others）, and change count into '1' and perMoney into '010'，no matter the web or ther server would get the same result as count '10' and perMoney into '10'
so the server would beileve that the paramater is correct and use it as usuall, this would do harm to the money or some logic of them

This sounds interesting. Do you have a reference for this type of attack?

yeah, we have some real case of this type of attack (but in chinese),
but i don't the exactly name of this type of attack in english,
so i am spending times on finding the same of this type of attack in english

Can't find a better reference than https://crypto.stackexchange.com/questions/55162/best-way-to-hash-two-values-into-one at the moment

@smowton can't agree any more

Why not just source = f1.getACall()?

haha, i am jsut new to here, not so familiar with it, so sometimes write some rules that could work but so ugly
thanks for your suggestions, i will update it quickly,

However, I think it would be even better to express this using data flow. Then you don't need to reason about a1 at all: DataFlow::localFlow(call1, call2.getReceiver()).

Similar to above: use Variable or data flow.

Message doesn't match header

what about this
Allowing Access-Control-Allow-Credentials as true would expand you attack surface if CORS attacks appears

You can pull this out rather than restate it in each or

Allowing cross-origin HTTP methods

Not necessary for this PR, but something to try out in the future: using the dataflow framework means we can pick up cases where either the header-name or the "*" isn't directly specified as a string constant, but gets there indirectly, through variables or function parameters / return values.

You could also use getAnElement().(KeyValueExpr) and then examine its getKey() and getValue() methods, to avoid having to explicitly specify the integer x at all.

I think this is a better idea than what I suggested above.

I think you probably want Method, a subclass of Function, and its three-argument hasQualifiedName function

This concept applies just as well for different hash algorithms

yes, but in most code, i found they use sha256. Later could expand the range of the hash algorithms

Agreed; no need to do the generalisation for the initial experimental query.

Rather than specify these exact sources, you might want to use UntrustedFlowSource, a class representing generally data supplied by the user.

Good idea, but i have no idea that UntrustedFlowSource whether include this sources of framework "beego", so i just specify the sources by myself,
Later i would rewrite it with UntrustedFlowSource and test it

We do not have a built-in model for beego at the moment. Leaving this as-is and generalising later is probably fine.

Can't find a better reference than https://crypto.stackexchange.com/questions/55162/best-way-to-hash-two-values-into-one at the moment

No need for this level of trickery. You can simply do

and get rid of tmp.

awesome！ i try many times using exists or any or for，but i could not fix this problem，so i have to use the tmp

to autoformat the ql file, using codeql query format file?

That's right, yes.

(And don't forget to pass -i to make it update the file in place.)

Now that you are using localFlow you don't need a1 above any more.

It doesn't look like call2 is used in the definition of source. Should it perhaps be used in some way?

As above, using localFlow means you don't need to reason about the assignment a1 any more, I think you can just remove it.

Slightly awkward name; perhaps change to something like

What is the role of c1? It does not seem to have anything to do with source.

Interesting! i deleted c1 and found it work as usual
At first, i use c1 to ensure the code did something to calc the hash, like
hashCalc := sha256.New()
hashCalc.Write(message_2)
In Go, you must declare it, and then use it's Write method

but by just deleting the line23 , i found it work as usual,
thanks

So are you planning on deleting this line?

i am not sure it's suitable or not to delete this line, because during my test (with code i have been saw and i thought out), it still works if i delete it.
i have no idea it would work well or not if you delete it in others situation,.
but in seriously, i think it should not be deleted if you want it run in critial

I'm not sure what you mean. This line is almost useless. The only thing it does is to ensure that there is a call to Write somewhere in the program. However, that call is not required to be related to source in any way at all. That doesn't sound like a very useful thing to check.

Why getASuccessor()?

yeah, this is something different between using d and d.getASuccessor()
here is the result of using d
you would found there is two kind of result call to xxx and the varialbe using Getxxx of beego to get user input
In this way, you would found the hole life circle of the variable, but i think this is a lttile complex
because in some situation like having so many filter function with the user's input
you would find so many result with the query
and what i want is to focus the first entrance of it , and i found getASuccessor() work, so i use it
and the result is like the second photo
(if necessary, i could change it into just using d)

I'm not entirely sure I understand, but are you saying that you think there will be too many sources if you omit the getASuccessor? Have you observed this to actually be a problem?

I think it might be easiest to just define source = m.getACall() and omit d entirely. The global data-flow will follow any steps that localFlow follows anyway.

sounds great, let me try

So are you planning on deleting this line?